Skip to content
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

SAM INIT support for custom runtimes #3726

Merged
merged 9 commits into from
Apr 7, 2022
Merged

Conversation

jonife
Copy link
Contributor

@jonife jonife commented Mar 12, 2022

Which issue(s) does this change fix?

Why is this change necessary?
This pr help us provide support for custom runtime in SAMCLI

How does it address the issue?
Adding this support will enable us to custom runtimes

What side effects does this change have?
None

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.

Copy link
Contributor

@mildaniel mildaniel left a comment

Choose a reason for hiding this comment

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

Thanks @jonife! Since this is more of a complete feature, could we also add an integration test(s) here too?

@@ -263,8 +269,10 @@ def _get_app_template_properties(
InvalidInitOptionException
exception raised when invalid option is provided
"""
# breakpoint()
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you might've forgotten to remove this line.

@@ -192,6 +197,7 @@ def _generate_from_use_case(
[*] Test Function in the Cloud: sam sync --stack-name {{stack-name}} --watch
"""
click.secho(next_commands_msg, fg="yellow")
runtime = get_custom_runtime_base_runtime(runtime) if is_custom_runtime(runtime) else runtime
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we define this value once above and re-use it avoid duplication of this line and line 173?

"""
if not runtime:
return False
validation_result = get_custom_runtime_base_runtime(runtime)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
validation_result = get_custom_runtime_base_runtime(runtime)
provided_runtime = get_custom_runtime_base_runtime(runtime)

return runtime in PROVIDED_RUNTIMES or bool(validation_result in PROVIDED_RUNTIMES)


def get_custom_runtime_base_runtime(runtime):
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def get_custom_runtime_base_runtime(runtime):
def get_provided_runtime_from_custom_runtime(runtime):

bool
True if there is a match else False
"""
if is_custom_runtime(filter_value) and filter_value != get_custom_runtime_base_runtime(template_runtime):
Copy link
Contributor

Choose a reason for hiding this comment

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

is_custom_runtime is already calling get_custom_runtime_base_runtime in it, why do we need to call it here again?

Copy link
Contributor Author

@jonife jonife Mar 30, 2022

Choose a reason for hiding this comment

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

we have both provided.al2 and provided, if a customer wants provided.al2 SAM INIT filters all runtimes and even down to the custom runtime's base runtime and generate a list of only provided.al2 runtimes which should be the expected behavior.

Therefore knowing that the runtime is a custom runtime isn't enough, digging deeper to get the customer the right custom runtime is what we want here. Just like its shown below, it narrows the runtime result list down to the exact provided runtime the customer wants:

(samcli39) jonife@147dda19a1dc python39x % samdev init -r provided.al2
Which template source would you like to use?
        1 - AWS Quick Start Templates
        2 - Custom Template Location
Choice: 1

Based on your selections, the only Template available is Hello World Example.
We will proceed to selecting the Template as Hello World Example.

Which runtime would you like to use?
        1 - graalvm.java11 (provided.al2)
        2 - rust (provided.al2)
Runtime:

Copy link
Contributor

@mndeveci mndeveci left a comment

Choose a reason for hiding this comment

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

Thanks a lot for addressing earlier feedback! Added some small comments there.

@@ -179,7 +181,8 @@ def get_preprocessed_manifest(
# at the top of list template example displayed to the Customer.
preprocessed_manifest = {"Hello World Example": {}} # type: dict
for template_runtime in manifest_body:
if filter_value and filter_value != template_runtime:
if not filter_value_matches_template_runtime(filter_value, template_runtime):
LOG.debug("Template runtime {template_runtime} does not match filter value {filter_value}")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
LOG.debug("Template runtime {template_runtime} does not match filter value {filter_value}")
LOG.debug("Template runtime %s does not match filter value %s", template_runtime, filter_value)

@mndeveci mndeveci merged commit 015dc05 into aws:develop Apr 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants