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

Changed autogen_policy default value from True to None #367

Merged
merged 5 commits into from
Jun 8, 2017

Conversation

stealthycoin
Copy link
Contributor

@stealthycoin stealthycoin commented Jun 5, 2017

Fixes #354

cc @kyleknap @jamesls

closes #281

@stealthycoin stealthycoin requested review from jamesls and kyleknap June 5, 2017 19:53
@codecov-io
Copy link

codecov-io commented Jun 5, 2017

Codecov Report

Merging #367 into master will increase coverage by 0.13%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #367      +/-   ##
==========================================
+ Coverage   90.73%   90.87%   +0.13%     
==========================================
  Files          18       18              
  Lines        2148     2148              
  Branches      276      276              
==========================================
+ Hits         1949     1952       +3     
+ Misses        146      145       -1     
+ Partials       53       51       -2
Impacted Files Coverage Δ
chalice/cli/__init__.py 79.27% <ø> (ø) ⬆️
chalice/cli/factory.py 89.79% <100%> (+3.06%) ⬆️

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 78bcccf...1476591. Read the comment docs.

@jamesls
Copy link
Member

jamesls commented Jun 6, 2017

There's also another PR for this: https://github.com/awslabs/chalice/pull/281/files

It removes the default of True in the CLI command. Do we need that here as well?

jcarlyl added 4 commits June 6, 2017 15:03
Updated some test related to the autogen policy on a deployment. Added
tests to ensure that the config file value is taken over the default if
no autogen-policy is specified on the command line.
@stealthycoin stealthycoin force-pushed the bugfix-autogen-policy branch from 9b26bf6 to 4b40375 Compare June 7, 2017 21:41
Copy link
Contributor

@kyleknap kyleknap left a comment

Choose a reason for hiding this comment

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

Looks good. Like the approach. I think it would also be worth to add the following test cases if there is not any:

  1. The default value for autogen_policy when nothing is provide to the create_config_obj()

  2. Make sure you can manually override whatever is set in the config.json by providing it straight to create_config_obj()

CHANGELOG.rst Outdated
@@ -9,6 +9,9 @@ Next Release (TBD)
redeployments of chalice applications and in the CloudFormation template
generated by ``chalice package``
(`#339 <https://github.com/awslabs/chalice/issues/339>`__)
* Fix autogen_policy in config being ignored
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably want to make autogen_policy be a code literal in the changelog.

@stealthycoin
Copy link
Contributor Author

Added those tests cases.

CHANGELOG.rst Outdated
@@ -9,7 +9,7 @@ Next Release (TBD)
redeployments of chalice applications and in the CloudFormation template
generated by ``chalice package``
(`#339 <https://github.com/awslabs/chalice/issues/339>`__)
* Fix autogen_policy in config being ignored
* Fix `autogen_policy` in config being ignored
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh it's rst not markdown. That gets me as well. Probably want to use `` instead of `

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops

Copy link
Contributor

@kyleknap kyleknap left a comment

Choose a reason for hiding this comment

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

Looks good. Just a small comment about the code literal. 🚢

@stealthycoin stealthycoin force-pushed the bugfix-autogen-policy branch from ae3ab22 to 1476591 Compare June 8, 2017 16:49
@stealthycoin stealthycoin merged commit 3616832 into aws:master Jun 8, 2017
@stealthycoin stealthycoin deleted the bugfix-autogen-policy branch June 8, 2017 17:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

autogen_policy from config.json is ignored when doing a chalice package
4 participants