Skip to content

feat: Add click command for cloud invoke command - #5238

Merged
hnnasit merged 16 commits into
aws:developfrom
hnnasit:remote-invoke-command
Jun 5, 2023
Merged

feat: Add click command for cloud invoke command#5238
hnnasit merged 16 commits into
aws:developfrom
hnnasit:remote-invoke-command

Conversation

@hnnasit

@hnnasit hnnasit commented Jun 1, 2023

Copy link
Copy Markdown
Contributor

Which issue(s) does this change fix?
#5018

This PR adds the click command for the new feature called cloud invoke. The formatting of the UX will be done in a separate PR.

What side effects does this change have?

N/A

Mandatory Checklist

PRs will only be reviewed after checklist is complete

  • Add input/output type hints to new functions/methods
  • Write design document if needed (Do I need to write a design document?)
  • Write/update unit tests
  • Write/update integration tests
  • Write/update functional tests if needed
  • make pr passes
  • make update-reproducible-reqs if dependencies were changed
  • Write documentation

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

@hnnasit
hnnasit requested a review from a team as a code owner June 1, 2023 15:38
@hnnasit
hnnasit requested review from hawflau and mildaniel June 1, 2023 15:38
Comment thread samcli/commands/_utils/options.py Outdated
Comment thread samcli/cli/types.py Outdated
Comment thread samcli/cli/types.py
Comment thread samcli/commands/remote_invoke/cloud.py Outdated
Comment thread samcli/commands/remote_invoke/cloud.py Outdated
Comment thread samcli/commands/remote_invoke/invoke/cli.py Outdated
Comment thread samcli/commands/remote_invoke/invoke/cli.py Outdated
Comment thread samcli/commands/remote_invoke/invoke/cli.py Outdated
Comment thread samcli/commands/remote_invoke/invoke/cli.py
Comment thread samcli/lib/cli_validation/event_file_validation.py Outdated
hnnasit and others added 7 commits June 1, 2023 10:13
Co-authored-by: Mehmet Nuri Deveci <5735811+mndeveci@users.noreply.github.com>
Co-authored-by: Mehmet Nuri Deveci <5735811+mndeveci@users.noreply.github.com>
Co-authored-by: Mehmet Nuri Deveci <5735811+mndeveci@users.noreply.github.com>
Co-authored-by: Mehmet Nuri Deveci <5735811+mndeveci@users.noreply.github.com>
@hnnasit
hnnasit requested a review from mndeveci June 2, 2023 04:16
Comment thread samcli/cli/types.py Fixed
Comment thread samcli/cli/types.py Fixed
Comment thread samcli/commands/remote_invoke/cloud.py
Comment thread samcli/lib/cli_validation/remote_invoke_options_validations.py
Comment thread samcli/cli/types.py
Comment thread samcli/cli/types.py
Comment thread samcli/commands/_utils/options.py Outdated
@configuration_option(provider=TomlProvider(section="parameters"))
@click.option("--stack-name", required=False, help="Name of the stack to get the resource information from")
@click.option("--resource-id", required=False, help="Name of the resource that will be invoked")
@click.option(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

we need similarly named options to have homogenity between local and cloud invoke?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this is something we will be discussing with the UX team next week. I will raise a follow up PR for the new UX changes to the help text and can add any named option changes there.

return resource_summary

@property
def stdout(self) -> StreamWriter:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should this be more common than here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I added it to the context because if we decide to add another handle, for instance log_file option that sam local has, we can add that here directly after passing it to the context.

@hnnasit
hnnasit requested review from mndeveci and sriram-mv June 2, 2023 22:42

@sriram-mv sriram-mv left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

overall LGTM

@mndeveci mndeveci left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for addressing all the feedback. Left a comment about a class naming, I think we need to rename all modules & classes once we decide the command naming.

Comment thread samcli/cli/types.py
return {key: _value}


class RemoteInvokeBotoApiParameterType(click.ParamType):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sorry that I didn't see this one before, but we may just hold on to remote invoke and change it afterwards if we decide to move forward with cloud invoke name.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yup that's right, left it as remote invoke here to keep it consistent with other places that use remote invoke. We can refactor all modules/classes/variable names once we confirm the name to be used.

@hnnasit
hnnasit added this pull request to the merge queue Jun 5, 2023
Merged via the queue into aws:develop with commit dd1adb1 Jun 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants