feat: remote invoke context initial impl #5183
Conversation
mndeveci
changed the title
| f"please provide --resource-id to resolve ambiguity." | ||
| ) | ||
|
|
||
| raise NoResourceFoundForRemoteInvoke( |
There was a problem hiding this comment.
nit: should we do a if not resource_summaries here first.
There was a problem hiding this comment.
Since we already exhausted all other options above, I was just raising the exception here. If you think that would be useful, I can add a comment on top of it to state this is the case where we don't have any resource summaries at all.
| _boto_client_provider: BotoProviderType | ||
| _boto_resource_provider: BotoProviderType | ||
| _stack_name: Optional[str] | ||
| _resource_id: Optional[str] | ||
| _resource_summary: Optional[CloudFormationResourceSummary] |
There was a problem hiding this comment.
Curious: Why define this here? Is there some advantage this has?
There was a problem hiding this comment.
This makes it more explicit about all the fields and their types that will/may be instantiated in this class. Mypy doesn't need to implicitly find their types from assignments. I know it is more verbose but I personally find this way more readable :)
| if not self._stack_name and not self._resource_id: | ||
| raise InvalidRemoteInvokeParameters("Either --stack-name or --resource-id parameter should be provided") |
There was a problem hiding this comment.
Should this check be in the "cli" part instead of the context? More a nit/question on where the validation of parameters happens (assuming this are parameters)
There was a problem hiding this comment.
🤔 agreed on this one, this validation can be moved up to command.py through click. @hnnasit what do you think?
There was a problem hiding this comment.
I think that makes sense. We will also have another validation for payload and payload-file in command.py. We can move the above validation to cli as well.
There was a problem hiding this comment.
Regarding to this one, I feel weird when removing this validation since typing system can't guarantee one or the other field should have a value. After discussing with @hnnasit we decided to keep it here as well as adding it to the command.py implementation later.
Adds
RemoteInvokeContextclass implementation with following validations;stack_nameorresource_idis provided, it fails withInvalidRemoteInvokeParametersstack_nameis provided;AmbiguousResourceForRemoteInvokeNoResourceFoundForRemoteInvokeresource_idis provided;resource_idis a valid ARN from supported resource, continues executionresource_idis a validphysical_resource_idin a deployed stack, continues executionresource_idis an invalidphysical_resource_id(where it can't be find by CFN API call), fails withAmbiguousResourceForRemoteInvokeMandatory Checklist
PRs will only be reviewed after checklist is complete
make prpassesmake update-reproducible-reqsif dependencies were changedBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.