feat(publish): Add --semantic-version option to sam publish command #1020

paoptu023 · 2019-02-21T23:01:50Z

Description of changes:
Add --semantic-version option to sam publish command. It overwrites the SemanticVersion value in the original template metadata, and publishes application with the provided version number.

The use case is that customers can easily use the build number from their continuous deployment pipelines to publish new application versions, without manually updating the template.

Checklist:

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

paoptu023 · 2019-02-27T00:10:09Z

Can we get an ETA for when this PR will get reviewed?

sriram-mv · 2019-02-27T00:12:59Z

samcli/commands/publish/command.py

@@ -31,31 +33,40 @@
 """
 SHORT_HELP = "Publish a packaged AWS SAM template to the AWS Serverless Application Repository."
 SERVERLESSREPO_CONSOLE_URL = "https://console.aws.amazon.com/serverlessrepo/home?region={}#/published-applications/{}"
+SEMANTIC_VERSION_HELP = "Optional. The value provided here overwrites SemanticVersion in the template metadata."


are there any restrictions on what the version can be set to? or is it up to the user to setup any version?

It's up to the user, but if they pass in invalid version numbers, the API call would fail. I didn't include an explanation of valid version numbers here, because it's already in the metadata doc.

Do we catch the exception correct and provide a useful message to customers within the CLI?

Will add code to catch the exception and print user-friendly error message.

jfuss · 2019-02-27T00:56:01Z

samcli/commands/publish/command.py

+    # Overwrite SemanticVersion in template metadata when provided in command input
+    if semantic_version and SERVERLESS_REPO_APPLICATION in template_data.get(METADATA, {}):
+        template_data.get(METADATA).get(SERVERLESS_REPO_APPLICATION)[SEMANTIC_VERSION] = semantic_version
+        with open(template, 'w') as fp:


What is the expectation here? What happens when this is a template that is generated and isn't the exact source copy customers edit themselves?

The expectation is that the template used to publish will always be up to date, and the latest semantic version can be tracked in the template. In most cases it's not the source copy since the template needs to be packaged first, but modifying the source copy requires additional inputs (like path to the source template), even with that we can't ensure it's the "source". I think it's better to keep the command simple and just update the published template.

I am not crazy about writing this back to the template. Till now, none of the commands writes or updates the template that is being handled. Can we pass the semantic version to the publish_application instead? Customers using build will NEED to update the source template anyways, as running build will squash this change to the template. I would prefer a solution that matches the expectation of editing the source rather than possibly editing the generated sources.

Made the change.

jfuss · 2019-02-27T00:57:07Z

Please update the sam publish design doc on changes.

sriram-mv · 2019-02-28T19:06:16Z

samcli/commands/publish/command.py

-        return UserException(
-            "Your SAM template contains invalid S3 URIs. Please make sure that you have uploaded application "
-            "artifacts to S3 by packaging the template: 'sam package --template-file <file-path>'.")
+    if error_code == 'BadRequestException':


I'm not too fond of looking at the message of the exception to determine the exception we throw, I dont know if there is a better way to do this though.

The only way to tell whether this exception is about S3 or Version is from the message, because we throw general BadRequestException 😞: https://docs.aws.amazon.com/serverlessrepo/latest/devguide/applications-applicationid-versions-semanticversion.html.

Are these the only ones we need to be catching? What is thrown when the semver already exists? Is throttle a concern?

To @thesriram note, can this be handled within the publish_application call? Instead of consumers of the library having to deal with all these cases, you could expand the library to throw more friendly exceptions. This would result in cleaner code here and for any other consumers.

Makes sense, I'll handle the exceptions inside serverlessrepo, except the S3 URI one. I still want to keep it here since the error message would be specific to sam cli (telling people to run sam package), what do you think?

jfuss · 2019-03-05T14:30:41Z

samcli/commands/publish/command.py

+    # Overwrite SemanticVersion in template metadata when provided in command input
+    if semantic_version and SERVERLESS_REPO_APPLICATION in template_data.get(METADATA, {}):
+        template_data.get(METADATA).get(SERVERLESS_REPO_APPLICATION)[SEMANTIC_VERSION] = semantic_version
+        with open(template, 'w') as fp:


I am not crazy about writing this back to the template. Till now, none of the commands writes or updates the template that is being handled. Can we pass the semantic version to the publish_application instead? Customers using build will NEED to update the source template anyways, as running build will squash this change to the template. I would prefer a solution that matches the expectation of editing the source rather than possibly editing the generated sources.

jfuss · 2019-03-05T14:35:13Z

samcli/commands/publish/command.py

-        return UserException(
-            "Your SAM template contains invalid S3 URIs. Please make sure that you have uploaded application "
-            "artifacts to S3 by packaging the template: 'sam package --template-file <file-path>'.")
+    if error_code == 'BadRequestException':


Are these the only ones we need to be catching? What is thrown when the semver already exists? Is throttle a concern?

To @thesriram note, can this be handled within the publish_application call? Instead of consumers of the library having to deal with all these cases, you could expand the library to throw more friendly exceptions. This would result in cleaner code here and for any other consumers.

Vicky Wang and others added 2 commits February 21, 2019 00:43

feat(publish): Add --semantic-version option to sam publish command

8744703

Merge branch 'develop' into semantic-version

af424a2

sriram-mv reviewed Feb 27, 2019

View reviewed changes

jfuss reviewed Feb 27, 2019

View reviewed changes

Vicky Wang added 2 commits February 27, 2019 14:28

Address feedback

e087eff

Update sam publish design doc

0f6a665

sriram-mv reviewed Feb 28, 2019

View reviewed changes

jfuss requested changes Mar 5, 2019

View reviewed changes

Vicky Wang added 2 commits March 5, 2019 18:29

Remove error message parsing and do not write semver to template

ad7fc53

Use override in help text

f452d2e

jfuss approved these changes Mar 7, 2019

View reviewed changes

sriram-mv self-requested a review March 7, 2019 18:29

sriram-mv approved these changes Mar 7, 2019

View reviewed changes

jfuss added 2 commits March 8, 2019 07:55

Merge branch 'develop' into semantic-version

9b05f4a

Merge branch 'develop' into semantic-version

3c19173

jfuss merged commit 43564aa into aws:develop Mar 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(publish): Add --semantic-version option to sam publish command #1020

feat(publish): Add --semantic-version option to sam publish command #1020

paoptu023 commented Feb 21, 2019

paoptu023 commented Feb 27, 2019

sriram-mv Feb 27, 2019

paoptu023 Feb 27, 2019

jfuss Feb 27, 2019

paoptu023 Feb 27, 2019

jfuss Feb 27, 2019

paoptu023 Feb 27, 2019

jfuss Mar 5, 2019

paoptu023 Mar 6, 2019

jfuss commented Feb 27, 2019

sriram-mv Feb 28, 2019

paoptu023 Feb 28, 2019

jfuss Mar 5, 2019

paoptu023 Mar 5, 2019

jfuss Mar 5, 2019

jfuss Mar 5, 2019

feat(publish): Add --semantic-version option to sam publish command #1020

feat(publish): Add --semantic-version option to sam publish command #1020

Conversation

paoptu023 commented Feb 21, 2019

paoptu023 commented Feb 27, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jfuss commented Feb 27, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment