SAM Publish add flag to fail on duplicate semantic version #6470
Conversation
github-actions
bot
added
area/publish
| def __init__(self, message=None, **kwargs): | ||
| """Init the exception object.""" | ||
| Exception.__init__(self, self.MESSAGE.format(**kwargs)) | ||
| message = self.MESSAGE.format(**kwargs) if message is None else message | ||
| Exception.__init__(self, message) |
There was a problem hiding this comment.
This is to allow publish exceptions to be created with dynamic messages instead of hardcoded in the class
| sar_client.get_application(ApplicationId=application_id, SemanticVersion=semantic_version) | ||
| return True | ||
| except ClientError as error: | ||
| if error.response["Error"]["Code"] == "NotFoundException": |
There was a problem hiding this comment.
Should we consider using .get() here instead of indexing?
There was a problem hiding this comment.
Yeah I just kept this consistent with what was already written. I don't think it's necessary given the set structure of the ClientError exception. We also see indexing done in the official ClientError docs
| def _check_app_with_semantic_version_exists(sar_client, application_id, semantic_version): | ||
| """ | ||
| Checks if a given SAR application exists with a given semantic version | ||
|
|
||
| :param sar_client: The boto3 client used to access SAR | ||
| :type sar_client: boto3.client | ||
| :param application_id: Application Id to check | ||
| :type sar_client: str | ||
| :param semantic_version: The semantic version to check with Application Id | ||
| :type sar_client: str | ||
| :return: Whether or not the given Application exists with the given semantic version | ||
| :rtype: bool | ||
| :raises ClientError | ||
| """ |
There was a problem hiding this comment.
nit: bool return type for the function signature, also looks like some of the types are duplicated.
As an aside, could we also update the type hints for this entire file since we are here to follow the the docstrings we've been using lately (ie Parameters/Returns/Raises)?
There was a problem hiding this comment.
Yeah my bad, copy paste errors.
Rewriting the typehints shouldn't take too long, and I think it's worth it. Honestly that format is atrocious.
| "--fail-on-same-version is set, but no semantic version is specified.\n" | ||
| "Please provide a semantic version in either the " | ||
| "template metadata or with the --semantic-version option." |
There was a problem hiding this comment.
Since the --fail option is SAM CLI specific, should move parts or all of this message over to SAM CLI instead of the vendor lib? Don't have a particularly strong preference to either
There was a problem hiding this comment.
I think if the vendor lib is a part of sam cli now, and it's not going anywhere, this should be fine here?
| if application_exists: | ||
| raise DuplicateSemanticVersionError( | ||
| f"Cannot publish version {semantic_version} for application " | ||
| "{application_id} because it already exists" |
There was a problem hiding this comment.
Missing the leading f for the f-string here
| @@ -222,6 +225,10 @@ def test_publish_existing_application_should_update_application_if_version_exist | |||
| self.serverlessrepo_mock.create_application.assert_called_once() | |||
| self.serverlessrepo_mock.update_application.assert_called_once() | |||
| self.serverlessrepo_mock.create_application_version.assert_called_once() | |||
| # If --fail-on-same-version is specified, warning is printed | |||
There was a problem hiding this comment.
This is the inverse right? If its not specified, we should expect to see a warning, but if we wanted to test that it publishes anyways, should we instead test if the _check_app_with_semantic_version_exists() method was not called, instead of testing log output?
There was a problem hiding this comment.
Sorry, my comment is wrong here. Should be # If --fail-on-same-version is NOT specified, warning is printed
I think we can keep the check for the log output, since the warning is important, but we can also check for _check_app_with_semantic_version_exists is not called
Which issue(s) does this change fix?
#6301
Why is this change necessary?
SAM Publish will succeed in updating a SAR application, even if the semantic version already exists.
This can cause issues for deployment, when CloudFormation will not recognize the updates since the semantic version has not changed.
How does it address the issue?
--fail-on-same-versionthat hard fails (instead of the warning) if a customer attempts to publish changes to an already existing semantic version.What side effects does this change have?
N/A
Mandatory 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.