Add --stage argument to chalice commands #270

jamesls · 2017-03-30T17:58:49Z

This pr builds on #264
to provide a consistent CLI experience when working with chalice stages.

There's several changes in this PR.

First, the code now clearly indicates whether we mean "chalice stage", which is
a completely separate collection of AWS resources, or an "API Gateway stage"
which is a specific configuration within a chalice stage (it's essentially the
URL prefix to use for your REST API for now). chalice_stage_name is
generally used when we mean a chalice stage and api_gateway_stage is used
when we mean the latter, and with the exception of the actual CLI option
(--stage) every existing reference to a "stage" has been renamed to one
of those two values.

Next, I've deprecated the form "chalice deploy ". You now get an error
message if you use this form which instructs you use to use
--api-gateway-stage:

$ chalice deploy prod
You've specified a deploy command of the form 'chalice deploy <stage>'
This form is deprecated and will be removed in a future version of chalice.
You can use the --api-gateway-stage to achieve the same functionality, or the newer '--stage' argument if you want an entirely set of separate resources.

To get the new stage functionality introduced in #264 you need to use
chalice deploy --stage prod.

And finally, I've added a --stage option to the remaining set of chalice
commands that should be stage specific:

url
logs
generate-sdk
package

All of these commands have also been updated to pull the deployed values
from the .chalice/deployed.json file instead of trying to query it using
the SDK as it was doing previously.

This is used to track when major features are introduced in the config file. Version is missing in existing config files so it will be assumed to be 1.0, hence the starting value of "2.0" in this commit.

This refactoring makes it easier to write CLI tests for all the various commands.

Remove all references to a plain "stage_name". Stage names are now explicit. Either you mean "API Gateway stage name" or "chalice stage name".

This allows for some configuratio to be specified as a top level key in .chalice/config.json as well as a chalice stage specific value. Now that chalice stages are completely separate sets of resources, you can have different config values per stage.

The "chalice deploy <stage>" is deprecated. Instead, I've added two options in its place: * --api-gateway-stage, for users that want the existing behavior * --stage, for the new stage behavior which creates an entirely separate set of AWS resources. I've also added a deprecation warning if you specify a positional stage as well as param validation if you specify both the api gateway stage as well as the positional stage param. As part of this commit I've also updated the remaining commands that need to be stage aware: * package * url * generate-sdk * logs For each of these commands I've added a "--stage" option that uses the deployed values JSON file to look up the appropriate stage values.

stealthycoin · 2017-03-30T19:44:56Z

chalice/cli/factory.py

+            exception = Exception(
+                "Unable to import your app.py file: %s" % e
+            )
+            # exception.exit_code = 2


Whats this?

Old code I need to update. This code was originally from the cli init.py which used click exceptions to propagate exit codes.

stealthycoin · 2017-03-30T19:50:02Z

chalice/config.py

-        ]
+    def _chain_lookup(self, name, varies_per_chalice_stage=False):
+        # type: (str, bool) -> Any
+        all_dicts = [self._user_provided_params]


all_dicts is probably not the best name considering that another dict is added to it if varies_per_chalice_stage is true. How bout something like search_dicts?

codecov-io · 2017-03-30T20:33:02Z

Codecov Report

Merging #270 into package will increase coverage by 1.77%.
The diff coverage is 92.19%.

@@             Coverage Diff             @@
##           package     #270      +/-   ##
===========================================
+ Coverage    83.98%   85.75%   +1.77%     
===========================================
  Files           17       17              
  Lines         1592     1664      +72     
  Branches       196      201       +5     
===========================================
+ Hits          1337     1427      +90     
+ Misses         202      184      -18     
  Partials        53       53

Impacted Files	Coverage Δ
chalice/package.py	`97.1% <100%> (ø)`	⬆️
chalice/awsclient.py	`82.14% <100%> (+0.12%)`	⬆️
chalice/constants.py	`100% <100%> (ø)`
chalice/deploy/deployer.py	`73.43% <100%> (ø)`	⬆️
chalice/cli/factory.py	`88.76% <88.76%> (ø)`
chalice/config.py	`97.29% <96.15%> (+6.38%)`	⬆️
chalice/__init__.py	`75.14% <0%> (-24.86%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7607b22...82e4005. Read the comment docs.

kyleknap · 2017-03-30T21:27:08Z

chalice/cli/__init__.py

+            deprecated_api_gateway_stage is not None:
+        raise _create_deprecated_stage_error(api_gateway_stage,
+                                             deprecated_api_gateway_stage)
+    if api_gateway_stage is None and deprecated_api_gateway_stage is not None:


Shouldn't this just be deprecated_api_gateway_stage is not None?

Updated, that was before adding the mutually exclusive part validation so the first clause isn't needed anymore.

stealthycoin · 2017-03-30T22:12:27Z

chalice/cli/factory.py

+                               "Are you sure this is a chalice project?")
+        app_obj = self.load_chalice_app()
+        user_provided_params['chalice_app'] = app_obj
+        if autogen_policy is not None:


Why would autogen_policy be None?

I'm double checking on this, but I think this code is no longer relevant.

The idea a while back was to toggle between three states: not set, true, and false. This specific check was to check if the user explicitly set a true/false value and if so then add it to the dict of user provided params. The "not set" was needed because of config precedence. If this option was not set, then move on to the next dict in the _chain_lookup of the Config obj.

It looks like you could never set autogen_policy from the config file, only via the CLI.

I think the policy management and configuration could use a refresh. This would be a fairly large change (I think) so I'll address this in a separate PR.

Quicker feedback cycles.

kyleknap

Looks good. Just mainly small questions and comments

kyleknap · 2017-03-30T21:29:32Z

chalice/cli/__init__.py

+
+def _warn_pending_removal(deprecated_stage):
+    # type: (str) -> None
+    click.echo("You've specified a deploy command of the form "


Should this be to stderr?

Fixed. I updated all the error messages to print to stderr.

kyleknap · 2017-03-30T21:41:27Z

chalice/cli/__init__.py

@@ -201,11 +218,17 @@ def deploy(ctx, autogen_policy, profile, stage):
 @click.option('--include-lambda-messages/--no-include-lambda-messages',
              default=False,
              help='Controls whether or not lambda log messages are included.')
+@click.option('--stage', default='dev')


Are users able to set the default for stage to override dev as the default? If so how does that happen?

No, not right now. I'm hesitant to allow people to change the default to something like "prod". I'd prefer if you're deploying from a script you have to run --stage prod.

That makes sense to be explicit, especially for debugging purposes.

kyleknap · 2017-03-30T21:48:24Z

chalice/cli/__init__.py

+                   "region value in our ~/.aws/config file.")
+        return 2
+    except Exception as e:
+        click.echo(str(e))


Might want to print this to stderr as well.

kyleknap · 2017-03-30T21:59:38Z

chalice/config.py

+    the config.environment_variables would be::
+
+        {"TABLE": "foo", "S3BUCKET": "devbucket"}
+


And so prod would be?
{"TABLE": "prodtable", "S3BUCKET": "prodbucket"}

Yeah. I updated the docs.

kyleknap · 2017-03-30T22:04:37Z

chalice/config.py

-        ]
+    def _chain_lookup(self, name, varies_per_chalice_stage=False):
+        # type: (str, bool) -> Any
+        all_dicts = [self._user_provided_params]


Yeah or cfg_dicts_chain?

kyleknap · 2017-03-30T22:09:09Z

chalice/config.py

    def __init__(self,
+                 chalice_stage='dev',


There is a lot of hardcoded default dev values. Wonder if it makes sense to have it be a constant to import or required instead of optional in case it ever needs to be updated?

I created a constants.py and moved a few of the constants over to that module.

kyleknap · 2017-03-30T22:30:48Z

tests/functional/cli/test_factory.py

+    assert logging.getLogger('').level == logging.DEBUG
+
+
+def test_can_create_botocoreo_session_cli_factory(clifactory):


typo botocoreo --> botocore

kyleknap · 2017-03-30T22:57:24Z

chalice/cli/__init__.py

+# This is the version that's written to the config file
+# on a `chalice new-project`.  It's also how chalice is able
+# to know when to warn you when changing behavior is introduced.
+CONFIG_VERSION = '2.0'


We should maybe throw errors if we encounter a config version higher than that version of the chalice can support

Seems reasonable, I'll update.

Removes hardcoded stage name of dev throughout the code and consolidates to a single location.

jamesls · 2017-03-31T20:05:33Z

@kyleknap I believe all the feedback's been incorporated.

kyleknap

Looks good. I like the addition of the constants module.

Before it would just print the top level file where the error occurs. Now it tries to reconstruct the error message you'd see if you let a syntaxerror propagate. This should help in troubleshooting.

jamesls · 2017-03-31T21:05:46Z

Cool, also @stealthycoin I improved the error messages on import errors:

 $ chalice deploy
Unable to import your app.py file:

File "/private/tmp/error/errors/chalicelib/james.py", line 1
  this is a syntax error in chalicelib/james.py

SyntaxError: invalid syntax

jamesls · 2017-03-31T23:04:53Z

Rebased and merged in 76bd8e1

jamesls added 5 commits March 29, 2017 14:37

Introduce version in config file

6f95e31

This is used to track when major features are introduced in the config file. Version is missing in existing config files so it will be assumed to be 1.0, hence the starting value of "2.0" in this commit.

Move object creation to factory class

4b54a79

This refactoring makes it easier to write CLI tests for all the various commands.

Clarify APIG stage vs. chalice stage

c1f60a8

Remove all references to a plain "stage_name". Stage names are now explicit. Either you mean "API Gateway stage name" or "chalice stage name".

jamesls requested review from kyleknap, dstufft, stealthycoin and JordonPhillips March 30, 2017 17:58

stealthycoin reviewed Mar 30, 2017

View reviewed changes

kyleknap reviewed Mar 30, 2017

View reviewed changes

stealthycoin reviewed Mar 30, 2017

View reviewed changes

jamesls added 3 commits March 30, 2017 15:26

Rename all_dicts to search_dicts

3ebe373

Add option to skip slow tests

50883c2

Quicker feedback cycles.

Remove unused clause

13a0824

kyleknap reviewed Mar 30, 2017

View reviewed changes

stealthycoin approved these changes Mar 30, 2017

View reviewed changes

jamesls added 5 commits March 31, 2017 12:48

Raise exception on unsupported config version

0b8d179

Update error message to print to stderr

48f64af

Clarify docs on prod stage resolution

1e65fdb

Fix typo in test

621c8ff

Extract constants to separate module

05a043e

Removes hardcoded stage name of dev throughout the code and consolidates to a single location.

kyleknap approved these changes Mar 31, 2017

View reviewed changes

Improve error message on import errors

82e4005

Before it would just print the top level file where the error occurs. Now it tries to reconstruct the error message you'd see if you let a syntaxerror propagate. This should help in troubleshooting.

jamesls closed this Mar 31, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add --stage argument to chalice commands #270

Add --stage argument to chalice commands #270

jamesls commented Mar 30, 2017

stealthycoin Mar 30, 2017

jamesls Mar 30, 2017

stealthycoin Mar 30, 2017

jamesls Mar 30, 2017

codecov-io commented Mar 30, 2017 •

edited

Loading

kyleknap Mar 30, 2017

jamesls Mar 30, 2017

stealthycoin Mar 30, 2017

jamesls Mar 30, 2017

jamesls Mar 30, 2017

kyleknap left a comment

kyleknap Mar 30, 2017

jamesls Mar 31, 2017

kyleknap Mar 30, 2017

jamesls Mar 31, 2017

kyleknap Mar 31, 2017

kyleknap Mar 30, 2017

kyleknap Mar 30, 2017

jamesls Mar 31, 2017

kyleknap Mar 30, 2017

kyleknap Mar 30, 2017

jamesls Mar 31, 2017

kyleknap Mar 30, 2017

kyleknap Mar 30, 2017

jamesls Mar 31, 2017

jamesls commented Mar 31, 2017

kyleknap left a comment

jamesls commented Mar 31, 2017

jamesls commented Mar 31, 2017

		the config.environment_variables would be::

		{"TABLE": "foo", "S3BUCKET": "devbucket"}

		assert logging.getLogger('').level == logging.DEBUG


		def test_can_create_botocoreo_session_cli_factory(clifactory):

Add --stage argument to chalice commands #270

Add --stage argument to chalice commands #270

Conversation

jamesls commented Mar 30, 2017

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

codecov-io commented Mar 30, 2017 • edited Loading

Codecov Report

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

kyleknap 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

jamesls commented Mar 31, 2017

kyleknap left a comment

Choose a reason for hiding this comment

jamesls commented Mar 31, 2017

jamesls commented Mar 31, 2017

codecov-io commented Mar 30, 2017 •

edited

Loading