Skip to content

Update Greengrass V2 IPC models #450

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jun 23, 2022
Merged

Update Greengrass V2 IPC models #450

merged 2 commits into from
Jun 23, 2022

Conversation

MikeDombo
Copy link
Contributor

Issue #, if available:

Description of changes:

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@MikeDombo MikeDombo marked this pull request as draft June 20, 2022 18:41
@MikeDombo MikeDombo marked this pull request as ready for review June 22, 2022 18:12
Copy link
Contributor

@TwistedTwigleg TwistedTwigleg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some questions, but the majority looks good! There is a couple places where it looks like there might be missing functionality though?

@@ -5864,6 +6878,75 @@ namespace Aws
return m_operationModelContext.GetOperationName();
}

GetClientDeviceAuthTokenOperationContext::GetClientDeviceAuthTokenOperationContext(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is empty. Is this supposed to be empty? I would expect it to return something since, based on the function name GetClientDeviceAuthTokenOperationContext

Edit: Ah, I see now. It is calling the OperationModelContext function in the constructor of the function. Ideally I'd like a comment here mentioning that, as initially it looks just like an empty function. However, this is not a requirement, now that I know to look for it, it makes sense.

return GetClientDeviceAuthTokenResponse::s_allocateFromPayload(stringView, allocator);
}

Aws::Crt::ScopedResource<AbstractShapeBase> GetClientDeviceAuthTokenOperationContext::
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto for this one - seems kinda pointless since it does not do anything.

ClientConnection &connection,
const VerifyClientDeviceIdentityOperationContext &operationContext,
Aws::Crt::Allocator *allocator) noexcept
: ClientOperation(connection, nullptr, operationContext, allocator)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same general remark on the constructors: A comment would be nice but not required.

return m_operationModelContext.GetOperationName();
}

AuthorizeClientDeviceActionOperationContext::AuthorizeClientDeviceActionOperationContext(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same general remark on the constructors: A comment would be nice but not required.

});
}

AuthorizeClientDeviceActionOperation::AuthorizeClientDeviceActionOperation(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same general remark on the constructors: A comment would be nice but not required.

@MikeDombo
Copy link
Contributor Author

Absolutely all of this is autogenerated from our code generators which have not been modified. The methods which return nullptr or empty strings are correct because those operations do not have any streaming response for example, they only have an immediate response.

@TwistedTwigleg
Copy link
Contributor

Ah okay, that makes sense. Thank you for clarifying! I will resolve the comments on the functions that return nullptr and/or empty strings 👍

Copy link
Contributor

@TwistedTwigleg TwistedTwigleg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spoke with the team and this is good to go 👍

@TwistedTwigleg
Copy link
Contributor

Thank you for the changes! Merging into main...

@TwistedTwigleg TwistedTwigleg merged commit b90ff7d into main Jun 23, 2022
@TwistedTwigleg TwistedTwigleg deleted the gg-v2-2.6.0 branch June 23, 2022 19:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants