feat: sam package without awscli pre-installed #1437

sriram-mv · 2019-09-30T22:21:35Z

Will be making additional changes and adding tests.

rely on boto3 instead
port over code from aws cli

Issue #, if available:

Description of changes:

Checklist:

Write Design Document (Do I need to write a design document?)
Write unit tests
Write/update functional tests
Write/update integration tests
make pr passes
Write documentation

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

jfuss

Are we planning on refactoring this more later? I know this is only the start but we will be incurring some tech debt with this if we keep package and deploy isolated from the rest of what we have built in this code base.

jfuss · 2019-10-22T13:22:41Z

samcli/commands/package/command.py

+CLI command for "package" command
+"""
+
+from functools import partial


In the command.py classes, we should move all imports we can into functions. We did this in other commands to help with sam --help load time.

jfuss · 2019-10-22T13:24:20Z

samcli/commands/package/command.py

+"""
+
+
+@click.command("package", short_help=SHORT_HELP, context_settings={"ignore_unknown_options": True}, help=HELP_TEXT)


Why do we need the context_settings here?

jfuss · 2019-10-22T13:26:59Z

samcli/commands/package/command.py

+    profile,
+):
+
+    with PackageCommandContext(


Nit: I don't think you need Command in the name. For invoke and build we have InvokeContext and BuildContext.

jfuss · 2019-10-22T13:30:26Z

samcli/commands/package/exceptions.py

+        self.kwargs = kwargs
+
+
+class InvalidTemplatePathError(CloudFormationCommandError):


Should we be combining these with our existing exceptions? We have an InvalidSamTemplateException in samcli/commands/local/cli_common/user_exceptions.py

totally, needs refactoring.

jfuss · 2019-10-22T13:32:19Z

samcli/commands/package/exceptions.py

@@ -0,0 +1,67 @@
+"""
+Exceptions that are raised by sam package and sam deploy


Nit: These exceptions are under package but also apply to deploy. Should we have a higher level exception class that includes all user exceptions or ones shared across commands instead?

yep, needs refactoring.

jfuss · 2019-10-22T14:11:21Z

samcli/commands/package/package_context.py

+            msg = self.MSG_PACKAGED_TEMPLATE_WRITTEN.format(
+                output_file_name=output_file, output_file_path=os.path.abspath(output_file)
+            )
+            sys.stdout.write(msg)


Move to click.echo

jfuss · 2019-10-22T14:15:00Z

samcli/commands/package/package_context.py

+        )
+
+        template_path = self.template_file
+        if not os.path.isfile(template_path):


This check isn't needed if you expand the click.Path() or change it to click.File() in the --template-file option on the command.

https://click.palletsprojects.com/en/7.x/api/#click.Path

jfuss · 2019-10-22T14:35:17Z

samcli/commands/package/artifact_exporter.py

+
+from samcli.commands.package import exceptions
+from samcli.yamlhelper import yaml_dump, yaml_parse
+import jmespath


New dependency? We should be adding this to our base.txt file

jfuss · 2019-10-22T14:37:17Z

samcli/commands/package/artifact_exporter.py

+
+    if isinstance(url, str) and url.startswith("s3://"):
+
+        # Python < 2.7.10 don't parse query parameters from URI with custom


Is there needed anymore since we have stopped support for 2.7

good catch, its from the port, will remove this.

jfuss · 2019-10-22T14:42:32Z

samcli/commands/package/artifact_exporter.py

+
+
+class ServerlessFunctionResource(Resource):
+    RESOURCE_TYPE = "AWS::Serverless::Function"


In the help text of the command, should we be adding what resources are supported?

How do we keep this in sync with https://github.com/awslabs/aws-sam-cli/blob/develop/samcli/commands/_utils/template.py#L19

Can do that.

sriram-mv · 2019-10-23T01:43:57Z

Did some refactor and added the first integration test for sam package

- rely on boto3 instead - port over code from aws cli

- unit tests for package context class - removal old `test_package.py`

- add unit tests for s3 uploader

- refactor of the port from `aws-cli` for artifact exporter - add unit tests for artifact exporter

- added unit tests for the click wiring

- fix for smoke tests

- https://docs.python.org/3/library/tempfile.html

- remove unneeded variables - remove unneeded exceptions, inherit package specific exceptions from UserException - refactor resources and their respective properties enums to its own file. This file is cross-referenced across package and other commands. - include jmespath as explicit dependency - refactor unit tests as required based on code structure changes.

Usage: samdev package [OPTIONS] The SAM package command creates a zip of your code and dependencies and uploads it to S3. The command returns a copy of your template, replacing references to local artifacts with the S3 location where the command uploaded the artifacts. The following resources and their property locations are supported. Resource : AWS::ServerlessRepo::Application | Location : LicenseUrl Resource : AWS::ServerlessRepo::Application | Location : ReadmeUrl Resource : AWS::Serverless::Function | Location : CodeUri Resource : AWS::Serverless::Api | Location : DefinitionUri Resource : AWS::AppSync::GraphQLSchema | Location :DefinitionS3Location Resource : AWS::AppSync::Resolver | Location : RequestMappingTemplateS3Location Resource : AWS::AppSync::Resolver | Location : ResponseMappingTemplateS3Location Resource : AWS::AppSync::FunctionConfiguration | Location : RequestMappingTemplateS3Location Resource : AWS::AppSync::FunctionConfiguration | Location : ResponseMappingTemplateS3Location Resource : AWS::Lambda::Function | Location : Code Resource : AWS::ApiGatewayRestApi | Location : BodyS3Location Resource : AWS::ElasticBeanstalk::ApplicationVersion | Location : SourceBundle Resource : AWS::CloudFormation::Stack | Location : TemplateURL Resource : AWS::Serverless::Application | Location : Location Resource : AWS::Lambda::LayerVersion | Location : Content Resource : AWS::Serverless::LayerVersion | Location : ContentUri Resource : AWS::Glue::Job | Location : Command.ScriptLocation

- add one barebones test to check `sam package` works

- With AWS::Serverless::Function Resource

sriram-mv · 2019-10-24T17:49:33Z

Rebased to latest from develop

- rebased on top of develop which removes python2 support

jfuss

I still haven't gone through the tests. But looks really good over all.

Question:
Will this support PR add support for Globals in sam package?

jfuss · 2019-10-24T18:31:22Z

.pylintrc

@@ -221,7 +221,7 @@ notes=FIXME,XXX
 [SIMILARITIES]

 # Minimum lines number of a similarity.
-min-similarity-lines=6
+min-similarity-lines=12


code smell alert?

its just number of arguments being similair.

==samcli.commands.package.command:110 ==samcli.commands.package.package_context:47 template_file, s3_bucket, s3_prefix, kms_key_id, output_template_file, use_json, force_upload, metadata, region, profile, ): (duplicate-code) samcli/settings/__init__.py:1:0: R0801: Similar lines in 2 files ==samcli.commands.package.command:96 ==samcli.commands.package.package_context:47 template_file, s3_bucket, s3_prefix, kms_key_id, output_template_file, use_json, force_upload, metadata, (duplicate-code)

jfuss · 2019-10-24T19:42:21Z

samcli/commands/_utils/resources.py

+}
+
+
+def resources_generator():


Can you add doc strings for this? I having a hard time parsing what this is doing/what we would use it for

Done, will add.

jfuss · 2019-10-24T19:43:51Z

samcli/commands/_utils/template.py

-    "AWS::Lambda::LayerVersion": ["Content"],
-    "AWS::Serverless::LayerVersion": ["ContentUri"],
-}
+from samcli.commands._utils.resources import METADATA_WITH_LOCAL_PATHS, RESOURCES_WITH_LOCAL_PATHS


jfuss · 2019-10-24T19:52:22Z

samcli/commands/package/s3_uploader.py

+        Exception.__init__(self, msg)
+        self.kwargs = kwargs
+
+    fmt = "S3 Bucket does not exist. " "Execute the command to create a new bucket" "\n" "aws s3 mb s3://{bucket_name}"


Will \n work on windows?

jfuss · 2019-10-24T19:54:26Z

samcli/commands/package/s3_uploader.py

+        return result
+
+
+class ProgressPercentage:


Should be be using what we do for Layers instead: https://github.com/awslabs/aws-sam-cli/blob/develop/samcli/lib/utils/progressbar.py?

I need to investigate if click's progress bar has a callback function. Click's own Progressbar class seems to be in a private py file _term_ui_impl.py and it also takes an iterable upfront to determine progress, which we dont know in this case. So, might need retro-fitting to use it.

It is publicly exposed through __init__.py: https://github.com/pallets/click/blob/master/click/__init__.py#L36

jfuss · 2019-10-24T19:55:41Z

samcli/commands/package/s3_uploader.py

+        with self._lock:
+            self._seen_so_far += bytes_transferred
+            percentage = (self._seen_so_far / self._size) * 100
+            sys.stdout.write(


Should this be stdout or stderr?
#970

Good thought, will change to stderr

sriram-mv · 2019-10-24T19:59:25Z

https://gist.github.com/TheSriram/3e40fd50307ae52c433e2a46246340f4

sriram-mv

I still haven't gone through the tests. But looks really good over all.

Question:
Will this support PR add support for Globals in sam package?

In another PR. not this one.

- Additional metadata click parameter type - Added unit, integration tests for `--metadata`

- Dont allow list or dict in values portion of a metadata dict

- added tests for `AWS::ServerlessRepo::Application` Metadata field

- compare output template-file from `sam package` and `aws cloudformation package`

sriram-mv · 2019-10-25T22:21:07Z

Regression tests passing: https://gist.github.com/TheSriram/120aa167a88b17a8c2175c8bdfe04ffe

jfuss · 2019-10-26T00:02:21Z

> I still haven't gone through the tests. But looks really good over all.

   Question:
   Will this support PR add support for Globals in sam package?

In another PR. not this one.

That is what I figured but wanted to ask anyways.

jfuss · 2019-10-28T14:25:31Z

appveyor-windows.yml

@@ -54,6 +54,8 @@ install:
    - "venv\\Scripts\\activate"
    - "python --version"

+    # Install AWS CLI
+    - "pip install awscli"


Is this temporary until we have things ported and confident in the changes?

This will install AWS CLI into the same venv as SAM CLI. Is there any issue with that (version conflicts potentials?)? If this is not temporary, we should be moving this to its own venv and putting the cli onto the path.

Yeah, my thinking was to keep regression suite running till we have to make an explicit change in behavior in our cli wrt to the commands, installation in a separate venv does make sense though. will address that! 👍

jfuss · 2019-10-28T14:32:46Z

samcli/cli/types.py

+
+    _pattern = r"([A-Za-z0-9\"]+)=([A-Za-z0-9\"]+)"
+
+    name = ""


What is name used for?

It isnt used for anything, but a custom type requires a name attribute, otherwise the help text crashes.

jfuss · 2019-10-28T14:38:02Z

samcli/cli/types.py

+
+    name = ""
+
+    def convert(self, value, param, ctx):


Can you add some comments above key sections here to make this method easier to understand. Looks like you are trying to parse the json way first and if that fails try the KeyName1=string, KeyName2=string2.

Definitely, can add a docstring.

jfuss · 2019-10-28T14:46:34Z

samcli/commands/_utils/resources.py

+    """
+    for resource, locations in dict({**METADATA_WITH_LOCAL_PATHS, **RESOURCES_WITH_LOCAL_PATHS}).items():
+        for location in locations:
+            yield "\nResource : {resource} | Location : {location}\n".format(resource=resource, location=location)


"\nResource : {resource} | Location : {location}\n".format(resource=resource, location=location) could be f"\nResource : {resource} | Location : {location}\n" 🐍3!

Nice, will change it.

jfuss · 2019-10-28T14:48:23Z

samcli/commands/_utils/resources.py

+
+def resources_generator():
+    """
+    Generator to yield set of resources and their locations that are supported for package operations


Is this used for help text generation? Reading this I thought this was going to yield {(resource, location)} but instead its a string. Just making sure I understand this correctly.

yeah, its a string, but I can make it yield (resource, location) tuple instead and leave the construction of the string to a different function.

jfuss · 2019-10-28T14:50:22Z

samcli/commands/package/command.py

+SHORT_HELP = "Package an AWS SAM application."
+
+
+def resources_and_properties_help_string():


Why not just have resources_generator return the string or move the logic within resources_generator here?

Having a generator of resource, location would be useful elsewhere too, is my opinion.

jfuss · 2019-10-28T14:56:01Z

samcli/commands/package/command.py

+
+@click.command("package", short_help=SHORT_HELP, help=HELP_TEXT, context_settings=dict(max_content_width=120))
+@click.option(
+    "--template-file",


Should we just be expanding the --template option to include --template-file as well? This would allow customers to be consistent across SAM CLI?

click.option( "--template", "--template-file", "-t", default=_TEMPLATE_OPTION_DEFAULT_VALUE, type=click.Path(), envvar="SAM_TEMPLATE_FILE", callback=partial(get_or_default_template_file_name, include_build=include_build), show_default=True, help="AWS SAM template file", )

makes sense.

jfuss · 2019-10-28T14:59:12Z

samcli/commands/package/command.py

+        try:
+            package_context.run()
+        except OSError as ex:
+            raise UserException(str(ex))


Why is OSError a UserException? Should we have a more specific UserException Error here?

Its primarily to do with Permissions on template file and output template file.

sriram-mv

Thanks for the review!

- template can now be specified as `-t`, `--template-file` and `--template`. Future PR will standardize it across the codebase. Current commit aims to fix it only in `package` space. - exception handling on passing an unknown profile name - refactor catching of OSError within package_context.py - New exception class for package failures.

jfuss

Nothing major just a couple small comments. I did notice that all of the files are under sam/commands/package/. We try and keep all command specific code under the sam/commands and move all 'library' related modules to sam/lib. artifact_exported and s3_uploader seem more like libs. Shouldn't we move them to follow the rest of the codebase's patterns?

jfuss · 2019-10-29T15:08:13Z

appveyor-windows.yml

@@ -49,6 +49,9 @@ install:
    # Upgrade setuptools, wheel and virtualenv
    - "python -m pip install --upgrade setuptools wheel virtualenv"

+    # Install AWS CLI Globally via pip3
+    - "pip3 install awscli"


pip3 in not on windows (according to build output)

moving back to pip.

jfuss · 2019-10-29T15:09:05Z

appveyor.yml

@@ -56,7 +59,7 @@ for:
      - "pylint --rcfile .pylintrc samcli"

      # Runs only in Linux
-      - sh: "pytest -vv tests/integration"
+      - sh: "pytest -vv -n 4 tests/integration"


How does this work? Are all our tests able to do this? Will this cause further issues with timeouts and other things we have been seeing?

sh: won't run in windows. These lines can all be removed from the windows matrix.

Should have added this for the regression suite, not here. will move.

jfuss · 2019-10-29T15:12:27Z

samcli/cli/context.py

+        try:
+            boto3.setup_default_session(region_name=self._aws_region, profile_name=self._aws_profile)
+        except botocore.exceptions.ProfileNotFound as ex:
+            raise UserException(str(ex))


Should we have more explicit exceptions? All we will see in Telemetry is UserException is thrown (which happens in a ton of places).

Sure, can add an explicit exception.

jfuss · 2019-10-29T15:23:02Z

samcli/commands/package/command.py

+
+
+@click.command("package", short_help=SHORT_HELP, help=HELP_TEXT, context_settings=dict(max_content_width=120))
+# TODO(TheSriram): Move to template_common_option across aws-sam-cli


Why not just do it now?

This will touch the full surface of commands across build, local etc. I'd rather have a separate PR for that.

jfuss · 2019-10-29T15:30:05Z

tests/integration/package/test_package_command.py

+
+        process = Popen(command_list, stdout=PIPE)
+        process.wait()
+        self.assertTrue(process.returncode, 1)


Thought assertTrue only takes in one expression? Unless the 1 is getting set to the message. Might be better to either remove 1 or make an explicit message.

moved to assertNotEqual to 0.

jfuss · 2019-10-29T15:33:03Z

tests/integration/package/test_package_command.py

+            process_stdout = b"".join(process.stdout.readlines()).strip()
+
+            self.assertIn(
+                bytes(


You can pass encoding into bytes builtin.

jfuss · 2019-10-29T15:45:30Z

tests/regression/package/regression_package_base.py

+        command = [base]
+        if os.getenv("SAM_CLI_DEV") and base == "sam":
+            command = ["samdev"]
+        elif base == "aws":


Does aws work on windows?

we are installing via pip, so the executable is aws.exe and should work.

Currently we check for both .exe and .cmd for windows. https://github.com/awslabs/aws-sam-cli/blob/develop/samcli/lib/samlib/cloudformation_command.py#L43

jfuss · 2019-10-29T15:48:06Z

tests/regression/package/test_package_regression.py

+        with tempfile.NamedTemporaryFile(delete=False) as output_template_file_sam:
+            template_path = self.test_data_path.joinpath(template_file)
+            sam_command_list = self.get_command_list(
+                s3_bucket=self.s3_bucket.name,
+                template_file=template_path,
+                output_template_file=output_template_file_sam.name,
+            )
+            process = Popen(sam_command_list, stdout=PIPE)
+            process.wait()


Could be nice to have this abstracted. I know the regression suite is more short term, if you don't feel like it will really help in creating new tests until its removed than just ignore this comment.

- move regression checking logic to base class - address comments on exceptions, appveyor file, encoding

jfuss

Awesome work!!!

jfuss reviewed Oct 22, 2019

View reviewed changes

sriram-mv added 3 commits October 24, 2019 10:48

feat: sam package without awscli pre-installed

4f385ad

- rely on boto3 instead - port over code from aws cli

tests: port artifact exporter unit tests

83f048f

tests: add package command unit tests

7ab0feb

sriram-mv force-pushed the package_off_awscli branch from f823fe7 to 1c63e19 Compare October 24, 2019 17:48

sriram-mv added 13 commits October 24, 2019 10:49

refactor: package context class

d8f6fc6

- unit tests for package context class - removal old `test_package.py`

feat: s3 uploader changes

d0e6584

- add unit tests for s3 uploader

refactor: sam package - artifact exporter

9322b71

- refactor of the port from `aws-cli` for artifact exporter - add unit tests for artifact exporter

refactor: unit tests for package context class

877b800

feat: sam package click wiring

377f23e

- added unit tests for the click wiring

tests: fix unit tests on windows

8905605

- fix for smoke tests

tests: fixing temporary files usage for windows

eeaf040

- https://docs.python.org/3/library/tempfile.html

chore: change pylint line similiarity

4d86298

tests: seed integration tests for sam package

e023e1a

- add one barebones test to check `sam package` works

fix: resource naming for resources enum

e1fe278

tests: sam package integration tests

1c63e19

- With AWS::Serverless::Function Resource

sriram-mv changed the title ~~[WIP] feat: sam package without awscli pre-installed~~ feat: sam package without awscli pre-installed Oct 24, 2019

sriram-mv added 2 commits October 24, 2019 11:19

fix: port code base to remove python2 quirks

1122113

- rebased on top of develop which removes python2 support

tests: add all supported resources for sam package integ tests

29e9874

jfuss requested changes Oct 24, 2019

View reviewed changes

sriram-mv commented Oct 24, 2019

View reviewed changes

feat: sam package supports --metadata

01fe649

- Additional metadata click parameter type - Added unit, integration tests for `--metadata`

sriram-mv force-pushed the package_off_awscli branch 2 times, most recently from dbdcf79 to 5b023d4 Compare October 25, 2019 18:49

fix: stricter checking on --metadata flag on sam package

5b023d4

- Dont allow list or dict in values portion of a metadata dict

sriram-mv added 2 commits October 25, 2019 12:01

fix: missed integ test for sam package

980ada5

- added tests for `AWS::ServerlessRepo::Application` Metadata field

tests: regression test suite for sam package

038cd38

- compare output template-file from `sam package` and `aws cloudformation package`

jfuss mentioned this pull request Oct 25, 2019

Revert "Revert "fix: Respect zipped symlinks (#1140)"" #1482

Merged

6 tasks

jfuss mentioned this pull request Oct 28, 2019

Undocumented options in '--help' for 'sam package' #1280

Closed

jfuss requested changes Oct 28, 2019

View reviewed changes

sriram-mv commented Oct 28, 2019

View reviewed changes

sriram-mv force-pushed the package_off_awscli branch 2 times, most recently from 0bca997 to 1d83aa7 Compare October 28, 2019 22:30

jfuss requested changes Oct 29, 2019

View reviewed changes

sriram-mv added 2 commits October 29, 2019 11:56

refactor: regression test suite

74c5e17

- move regression checking logic to base class - address comments on exceptions, appveyor file, encoding

lint: linter fixes

d226e7f

jfuss approved these changes Oct 30, 2019

View reviewed changes

sriram-mv mentioned this pull request Oct 31, 2019

Deploy off aws cli #1455

Merged

6 tasks

sriram-mv merged commit 7cbcfff into aws:develop Nov 5, 2019

		"""


		@click.command("package", short_help=SHORT_HELP, context_settings={"ignore_unknown_options": True}, help=HELP_TEXT)

		self.kwargs = kwargs


		class InvalidTemplatePathError(CloudFormationCommandError):

		@@ -0,0 +1,67 @@
		"""
		Exceptions that are raised by sam package and sam deploy


		if isinstance(url, str) and url.startswith("s3://"):

		# Python < 2.7.10 don't parse query parameters from URI with custom



		class ServerlessFunctionResource(Resource):
		RESOURCE_TYPE = "AWS::Serverless::Function"

		SHORT_HELP = "Package an AWS SAM application."


		def resources_and_properties_help_string():



		@click.command("package", short_help=SHORT_HELP, help=HELP_TEXT, context_settings=dict(max_content_width=120))
		# TODO(TheSriram): Move to template_common_option across aws-sam-cli

feat: sam package without awscli pre-installed #1437

feat: sam package without awscli pre-installed #1437

Conversation

sriram-mv commented Sep 30, 2019 • edited Loading

jfuss left a 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

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

sriram-mv commented Oct 23, 2019

sriram-mv commented Oct 24, 2019

jfuss left a 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

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

sriram-mv commented Oct 24, 2019

sriram-mv left a comment

Choose a reason for hiding this comment

sriram-mv commented Oct 25, 2019

jfuss commented Oct 26, 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

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

sriram-mv left a comment

Choose a reason for hiding this comment

jfuss left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jfuss left a comment

Choose a reason for hiding this comment

sriram-mv commented Sep 30, 2019 •

edited

Loading