Deploy off aws cli #1455

sriram-mv · 2019-10-09T21:55:34Z

Clean output of stack events as deploy progress
Clean output of changeset before beginning the deploy process
Port from AWS CLI
Unit tests added
Integration tests added
Regression tests added
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.

sriram-mv · 2019-10-31T00:30:35Z

The number of commits appear huge, but thats because this is based off sam package PR: #1437

This is still very much in WIP.

sriram-mv · 2019-10-31T00:31:27Z

samcli/lib/deploy/deployer.py

@@ -140,7 +155,8 @@ def create_changeset(
            LOG.debug("Unable to create changeset", exc_info=ex)
            raise ex

-    def describe_changeset(self, change_set_id, stack_name):
+    @pprint(format_string=DESCRIBE_CHANGESET_FORMAT_STRING, format_kwargs=DESCRIBE_CHANGESET_DEFAULT_ARGS)


click based decorator to create a table.

- Not breaking parameter overrides formats - still requires refactoring and error handling

- needs refactoring

- wire up command.py for `sam deploy` - move deploy specific exceptions to inherit from UserException

- `sam deploy` now has tables while showcasing the changeset and showcasing events happening during deploy.

- fixed unit tests - linting fixes - doc strings - further unit tests and integration tests need to be added.

- exercise all command line parameters for `aws` and `sam`

samcli/cli/types.py

awood45 · 2019-11-11T18:20:35Z

samcli/commands/_utils/custom_options/option_nargs.py

+            value = [value]
+
+            while state.rargs and not next_option:
+                for prefix in self._nargs_parser.prefixes:


Do we have or intend to support any commands where there is a trailing argument that does not have a prefix?

For example, many CLIs have a valid command that works much like the following:

cli commandname --option-one A --option-set B C D --input inputfile.txt outputfile.txt

(Where outputfile.txt is a positional argument that goes at the end of the full command.)

This code would appear to assume we never allow for that pattern. Is this a correct assumption for us?

True, one way to codify that would be to add such positional argument to the front of the command. I'm not sure if there are alternate ways to check for unbounded number of arguments for a given option without a breaking change though.

awood45 · 2019-11-11T18:26:15Z

samcli/commands/deploy/command.py

+    "change  set.  Specify  this flag if you want to view your stack changes"
+    "before executing the change set. The command creates an AWS CloudForma-"
+    "tion  change set and then exits without executing the change set. After"
+    "you view the change set, execute it to implement your changes.",


Execute it how? We should make that clear somewhere, probably here.

will clarify this.

awood45 · 2019-11-11T20:16:51Z

samcli/lib/deploy/deployer.py

+        """
+        try:
+            resp = self._client.describe_stacks(StackName=stack_name)
+            if len(resp["Stacks"]) != 1:


I'd actually check that this is equal to zero.

I believe (could be wrong) that certain situations and inputs can lead this number to be greater than one. At the very least, the API documentation lists this as a possibility.

I'm looking at this as a defensive programming POV - I don't think the logic below needs to change.

sriram-mv · 2019-11-12T05:58:38Z

@awood45 addressed your comments.

awood45

One comment to verify before merging.

awood45 · 2019-11-12T21:44:28Z

samcli/lib/deploy/deployer.py

@@ -69,7 +69,7 @@ def has_stack(self, stack_name):
        """
        try:
            resp = self._client.describe_stacks(StackName=stack_name)
-            if len(resp["Stacks"]) != 1:
+            if not resp["Stacks"]:


Does this handle an empty array?

Actually, better question, can we ensure that we test the case of no such stack? Just to be sure for regressions.

sriram-mv · 2019-11-12T22:06:28Z

tests/unit/lib/deploy/test_deployer.py

+        self.assertEqual(self.deployer._client, self.cloudformation_client)
+        self.assertEqual(self.deployer.changeset_prefix, "samcli-cloudformation-package-deploy-")
+
+    def test_deployer_has_no_stack(self):


Example of such a test, @awood45

sanathkr

Most comments are minor, something to think about. Just make sure the table formatting doesn't lead to zeros or infinities or crashes. We don't want deploy to crash because of the table print.

samcli/lib/deploy/deployer.py

sanathkr · 2019-11-13T01:30:23Z

samcli/lib/deploy/deployer.py

+            try:
+                outputs = stacks_description["Stacks"][0]["Outputs"]
+                if echo:
+                    click.secho("\nStack {stack_name} outputs:\n".format(stack_name=stack_name))


Can you not use click in this layer? It adds a dependency on click for this module to function. It makes the code less portable.

samcli/lib/deploy/deployer.py

sanathkr · 2019-11-13T01:45:20Z

samcli/commands/_utils/table_print.py

+        usable_width_no_margin = int(width) - 1
+        usable_width = int((usable_width_no_margin - (margin if margin else min_margin)))
+
+        width_per_column = int(usable_width / total_args)


Can this be zero?

Can this be infinity?

added ValueError if so.

samcli/commands/_utils/table_print.py

samcli/lib/deploy/deployer.py

- add retries - more regression testing - remove types for capabilities

sanathkr

Thank you for making the changes. This looks good to me.

sanathkr · 2019-11-13T18:01:08Z

samcli/lib/deploy/deployer.py


    def _check_stack_complete(self, status):
        return "COMPLETE" in status and "CLEANUP" not in status

    def wait_for_execute(self, stack_name, changeset_type):

-        click.secho("\nWaiting for stack create/update to complete\n")
+        sys.stdout.write("\nWaiting for stack create/update to complete\n")


Are you sure these are written to stdout in AWS CLI?

https://github.com/aws/aws-cli/blob/develop/awscli/customizations/cloudformation/deployer.py#L154

I can add flushes. 👍

+1 add flushes

- show cases Add, Modify, Delete with +, * and -

* feat: sam deploy without aws cli pre-installed - Not breaking parameter overrides formats - still requires refactoring and error handling * feat: new click types for deploy parameters * feat: show changeset and stack events - needs refactoring * refactor: move deploy classes to lib - wire up command.py for `sam deploy` - move deploy specific exceptions to inherit from UserException * rebase: latest from `sam package` port * feat: decorator for printing tables - `sam deploy` now has tables while showcasing the changeset and showcasing events happening during deploy. * fix: wrap text on resource status column on `sam deploy` - fixed unit tests - linting fixes - doc strings - further unit tests and integration tests need to be added. * fix: cleaner text formatting for tables * tests: add unit tests for full suite of `sam deploy` * tests: add integration tests for `sam deploy` * tests: regression test suite for `sam deploy` - exercise all command line parameters for `aws` and `sam` * fix: deploy command now showcases stack outputs * fix: address comments * fix: return stack outputs from `get_stack_outputs` * fix: width margins on table prints * fix: address comments - add retries - more regression testing - remove types for capabilities * tests: tests for pprint of tables * usability: add table headers - show cases Add, Modify, Delete with +, * and -

This was referenced Oct 18, 2019

(feat) boto3 sam deploy #1323

Closed

sam deploy boto3 design doc #1324

Closed

sriram-mv force-pushed the deploy_off_aws_cli branch from 43ce53c to bbf1cca Compare October 31, 2019 00:29

sriram-mv commented Oct 31, 2019

View reviewed changes

sriram-mv added 8 commits November 5, 2019 18:07

feat: sam deploy without aws cli pre-installed

b93374e

- Not breaking parameter overrides formats - still requires refactoring and error handling

feat: new click types for deploy parameters

7fa2a9d

feat: show changeset and stack events

b16215b

- needs refactoring

refactor: move deploy classes to lib

8478dea

- wire up command.py for `sam deploy` - move deploy specific exceptions to inherit from UserException

rebase: latest from sam package port

dff6bc2

feat: decorator for printing tables

346ba1d

- `sam deploy` now has tables while showcasing the changeset and showcasing events happening during deploy.

fix: wrap text on resource status column on sam deploy

9fccb61

- fixed unit tests - linting fixes - doc strings - further unit tests and integration tests need to be added.

fix: cleaner text formatting for tables

787b270

sriram-mv force-pushed the deploy_off_aws_cli branch from bd1dc60 to 787b270 Compare November 6, 2019 02:11

sriram-mv added 2 commits November 6, 2019 18:58

tests: add unit tests for full suite of sam deploy

9c3f7d8

tests: add integration tests for sam deploy

e576ad1

sriram-mv changed the title ~~[WIP] : Deploy off aws cli~~ Deploy off aws cli Nov 7, 2019

sriram-mv added 2 commits November 7, 2019 00:08

tests: regression test suite for sam deploy

0a4691f

- exercise all command line parameters for `aws` and `sam`

fix: deploy command now showcases stack outputs

8d0052d

awood45 requested changes Nov 11, 2019

View reviewed changes

fix: address comments

5040144

jfuss added area/deploy sam deploy command type/feature Feature request labels Nov 12, 2019

sriram-mv added 2 commits November 12, 2019 12:22

fix: return stack outputs from get_stack_outputs

fe79bf1

fix: width margins on table prints

c6aa191

awood45 approved these changes Nov 12, 2019

View reviewed changes

sriram-mv commented Nov 12, 2019

View reviewed changes

sanathkr suggested changes Nov 13, 2019

View reviewed changes

fix: address comments

d08b5ea

- add retries - more regression testing - remove types for capabilities

tests: tests for pprint of tables

aa8cefc

sanathkr approved these changes Nov 13, 2019

View reviewed changes

usability: add table headers

17594b0

- show cases Add, Modify, Delete with +, * and -

sriram-mv changed the base branch from develop to release-v0.32.0 November 13, 2019 18:56

sanathkr approved these changes Nov 13, 2019

View reviewed changes

sriram-mv merged commit d51d6f7 into aws:release-v0.32.0 Nov 13, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deploy off aws cli #1455

Deploy off aws cli #1455

sriram-mv commented Oct 9, 2019 •

edited

Loading

sriram-mv commented Oct 31, 2019

sriram-mv Oct 31, 2019

awood45 Nov 11, 2019

sriram-mv Nov 12, 2019

awood45 Nov 11, 2019

sriram-mv Nov 12, 2019

sriram-mv Nov 12, 2019

awood45 Nov 11, 2019

awood45 Nov 11, 2019

sriram-mv Nov 12, 2019

sriram-mv commented Nov 12, 2019

awood45 left a comment

awood45 Nov 12, 2019

awood45 Nov 12, 2019

sriram-mv Nov 12, 2019

sanathkr left a comment

sanathkr Nov 13, 2019

sriram-mv Nov 13, 2019

sanathkr Nov 13, 2019

sriram-mv Nov 13, 2019

sanathkr left a comment

sanathkr Nov 13, 2019

sriram-mv Nov 13, 2019

sanathkr Nov 13, 2019

Deploy off aws cli #1455

Deploy off aws cli #1455

Conversation

sriram-mv commented Oct 9, 2019 • edited Loading

sriram-mv commented Oct 31, 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

sriram-mv commented Nov 12, 2019

awood45 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

sanathkr 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

sanathkr 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

sriram-mv commented Oct 9, 2019 •

edited

Loading