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

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

Merged
merged 8 commits into from
Mar 8, 2019
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 15 additions & 4 deletions samcli/commands/publish/command.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,14 @@
from botocore.exceptions import ClientError
from serverlessrepo import publish_application
from serverlessrepo.publish import CREATE_APPLICATION
from serverlessrepo.parser import METADATA, SERVERLESS_REPO_APPLICATION
from serverlessrepo.exceptions import ServerlessRepoError

from samcli.cli.main import pass_context, common_options as cli_framework_options, aws_creds_options
from samcli.commands._utils.options import template_common_option
from samcli.commands._utils.template import get_template_data
from samcli.commands.exceptions import UserException
from samcli.yamlhelper import yaml_dump

HELP_TEXT = """
Use this command to publish a packaged AWS SAM template to
Expand All @@ -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."
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

SEMANTIC_VERSION = 'SemanticVersion'


@click.command("publish", help=HELP_TEXT, short_help=SHORT_HELP)
@template_common_option
@click.option('--semantic-version', help=SEMANTIC_VERSION_HELP)
@aws_creds_options
@cli_framework_options
@pass_context
def cli(ctx, template):
def cli(ctx, template, semantic_version):
# All logic must be implemented in the ``do_cli`` method. This helps with easy unit testing

do_cli(ctx, template) # pragma: no cover
do_cli(ctx, template, semantic_version) # pragma: no cover


def do_cli(ctx, template):
def do_cli(ctx, template, semantic_version):
"""Publish the application based on command line inputs."""
try:
template_data = get_template_data(template)
except ValueError as ex:
click.secho("Publish Failed", fg='red')
raise UserException(str(ex))

# 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:
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made the change.

fp.write(yaml_dump(template_data))

try:
publish_output = publish_application(template_data)
click.secho("Publish Succeeded", fg="green")
click.secho(_gen_success_message(publish_output), fg="yellow")
click.secho(_gen_success_message(publish_output))
except ServerlessRepoError as ex:
click.secho("Publish Failed", fg='red')
raise UserException(str(ex))
Expand Down
5 changes: 4 additions & 1 deletion tests/integration/publish/publish_app_integ_base.py
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ def base_command(self):

return command

def get_command_list(self, template_path=None, region=None, profile=None):
def get_command_list(self, template_path=None, region=None, profile=None, semantic_version=None):
command_list = [self.base_command(), "publish"]

if template_path:
Expand All @@ -102,4 +102,7 @@ def get_command_list(self, template_path=None, region=None, profile=None):
if profile:
command_list = command_list + ["--profile", profile]

if semantic_version:
command_list = command_list + ["--semantic-version", semantic_version]

return command_list
21 changes: 21 additions & 0 deletions tests/integration/publish/test_command_integ.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

from unittest import skipIf

from samcli.commands.publish.command import SEMANTIC_VERSION
from .publish_app_integ_base import PublishAppIntegBase

# Publish tests require credentials and Travis will only add credentials to the env if the PR is from the same repo.
Expand Down Expand Up @@ -59,6 +60,26 @@ def test_create_application_version(self):
app_metadata = json.loads(app_metadata_text)
self.assert_metadata_details(app_metadata, process_stdout.decode('utf-8'))

def test_create_application_version_overwrite_template_semantic_version(self):
template_path = self.temp_dir.joinpath("template_create_app_version.yaml")
command_list = self.get_command_list(
template_path=template_path, region=self.region_name, semantic_version='0.1.0')

process = Popen(command_list, stdout=PIPE)
process.wait()
process_stdout = b"".join(process.stdout.readlines()).strip()

overwritten_template_text = template_path.read_text()
self.assertIn('{}: 0.1.0'.format(SEMANTIC_VERSION), overwritten_template_text)

expected_msg = 'The following metadata of application "{}" has been updated:'.format(self.application_id)
self.assertIn(expected_msg, process_stdout.decode('utf-8'))

app_metadata_text = self.temp_dir.joinpath("metadata_create_app_version.json").read_text()
app_metadata = json.loads(app_metadata_text)
app_metadata[SEMANTIC_VERSION] = '0.1.0'
self.assert_metadata_details(app_metadata, process_stdout.decode('utf-8'))


@skipIf(SKIP_PUBLISH_TESTS, "Skip publish tests in Travis only")
class TestPublishNewApp(PublishAppIntegBase):
Expand Down
67 changes: 56 additions & 11 deletions tests/unit/commands/publish/test_command.py
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
"""Test sam publish CLI."""
import json
from unittest import TestCase
from mock import patch, call, Mock
from mock import patch, call, Mock, mock_open

from botocore.exceptions import ClientError
from serverlessrepo.exceptions import ServerlessRepoError
from serverlessrepo.publish import CREATE_APPLICATION, UPDATE_APPLICATION
from serverlessrepo.parser import METADATA, SERVERLESS_REPO_APPLICATION

from samcli.commands.publish.command import do_cli as publish_cli
from samcli.commands.publish.command import do_cli as publish_cli, SEMANTIC_VERSION
from samcli.commands.exceptions import UserException


Expand All @@ -26,7 +27,7 @@ def setUp(self):
def test_must_raise_if_value_error(self, click_mock, get_template_data_mock):
get_template_data_mock.side_effect = ValueError("Template not found")
with self.assertRaises(UserException) as context:
publish_cli(self.ctx_mock, self.template)
publish_cli(self.ctx_mock, self.template, None)

message = str(context.exception)
self.assertEqual("Template not found", message)
Expand All @@ -38,7 +39,7 @@ def test_must_raise_if_value_error(self, click_mock, get_template_data_mock):
def test_must_raise_if_serverlessrepo_error(self, click_mock, publish_application_mock):
publish_application_mock.side_effect = ServerlessRepoError()
with self.assertRaises(UserException):
publish_cli(self.ctx_mock, self.template)
publish_cli(self.ctx_mock, self.template, None)

click_mock.secho.assert_called_with("Publish Failed", fg="red")

Expand All @@ -56,7 +57,7 @@ def test_must_raise_if_s3_uri_error(self, click_mock, publish_application_mock):
'create_application'
)
with self.assertRaises(UserException) as context:
publish_cli(self.ctx_mock, self.template)
publish_cli(self.ctx_mock, self.template, None)

message = str(context.exception)
self.assertIn("Please make sure that you have uploaded application artifacts "
Expand All @@ -72,7 +73,7 @@ def test_must_raise_if_not_s3_uri_error(self, click_mock, publish_application_mo
'other_operation'
)
with self.assertRaises(ClientError):
publish_cli(self.ctx_mock, self.template)
publish_cli(self.ctx_mock, self.template, None)

click_mock.secho.assert_called_with("Publish Failed", fg="red")

Expand All @@ -86,7 +87,7 @@ def test_must_succeed_to_create_application(self, click_mock, publish_applicatio
'actions': [CREATE_APPLICATION]
}

publish_cli(self.ctx_mock, self.template)
publish_cli(self.ctx_mock, self.template, None)
details_str = json.dumps({'attr1': 'value1'}, indent=2)
expected_msg = "Created new application with the following metadata:\n{}"
expected_link = self.console_link.format(
Expand All @@ -95,7 +96,7 @@ def test_must_succeed_to_create_application(self, click_mock, publish_applicatio
)
click_mock.secho.assert_has_calls([
call("Publish Succeeded", fg="green"),
call(expected_msg.format(details_str), fg="yellow"),
call(expected_msg.format(details_str)),
call(expected_link, fg="yellow")
])

Expand All @@ -109,7 +110,7 @@ def test_must_succeed_to_update_application(self, click_mock, publish_applicatio
'actions': [UPDATE_APPLICATION]
}

publish_cli(self.ctx_mock, self.template)
publish_cli(self.ctx_mock, self.template, None)
details_str = json.dumps({'attr1': 'value1'}, indent=2)
expected_msg = 'The following metadata of application "{}" has been updated:\n{}'
expected_link = self.console_link.format(
Expand All @@ -118,7 +119,7 @@ def test_must_succeed_to_update_application(self, click_mock, publish_applicatio
)
click_mock.secho.assert_has_calls([
call("Publish Succeeded", fg="green"),
call(expected_msg.format(self.application_id, details_str), fg="yellow"),
call(expected_msg.format(self.application_id, details_str)),
call(expected_link, fg="yellow")
])

Expand All @@ -139,9 +140,53 @@ def test_print_console_link_if_context_region_not_set(self, click_mock, boto3_mo
session_mock.region_name = "us-west-1"
boto3_mock.Session.return_value = session_mock

publish_cli(self.ctx_mock, self.template)
publish_cli(self.ctx_mock, self.template, None)
expected_link = self.console_link.format(
session_mock.region_name,
self.application_id.replace('/', '~')
)
click_mock.secho.assert_called_with(expected_link, fg="yellow")

@patch('samcli.commands.publish.command.get_template_data')
@patch('samcli.commands.publish.command.publish_application')
def test_must_use_template_semantic_version(self, publish_application_mock,
get_template_data_mock):
template_data = {
METADATA: {
SERVERLESS_REPO_APPLICATION: {SEMANTIC_VERSION: '0.1'}
}
}
get_template_data_mock.return_value = template_data
publish_application_mock.return_value = {
'application_id': self.application_id,
'details': {}, 'actions': {}
}
publish_cli(self.ctx_mock, self.template, None)
publish_application_mock.assert_called_with(template_data)

@patch('samcli.commands.publish.command.get_template_data')
@patch('samcli.commands.publish.command.publish_application')
def test_must_overwrite_semantic_version_if_provided(self, publish_application_mock,
get_template_data_mock):
template_data = {
METADATA: {
SERVERLESS_REPO_APPLICATION: {SEMANTIC_VERSION: '0.1'}
}
}
get_template_data_mock.return_value = template_data
publish_application_mock.return_value = {
'application_id': self.application_id,
'details': {}, 'actions': {}
}

m = mock_open()
with patch("samcli.commands.publish.command.open", m):
publish_cli(self.ctx_mock, self.template, '0.2')

m.assert_called_with(self.template, 'w')
expected_template_data = {
METADATA: {
SERVERLESS_REPO_APPLICATION: {SEMANTIC_VERSION: '0.2'}
}
}
publish_application_mock.assert_called_with(expected_template_data)