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

Parse pricing API to get allowed values #586

Merged
merged 8 commits into from
Jan 25, 2019

Conversation

fatbasstard
Copy link
Contributor

@fatbasstard fatbasstard commented Jan 15, 2019

Took over the commit from @kddejong to make changes and improve the allowedValues rule using this: The first "region specific" allowedValues 🎉

Current status/changes:

  • API loads Ec2, AmazonMQ and RDS Instance types and created region specific valueType configurations (™️ @kddejong)
  • Adeed the new spec patches to the setup.py so they get packaged
  • Attached the Valuetypes to a bunch of resources and added them to the allowed_values tests
  • Changed the initialization flow of the rules. Instead of always using us-east-1, the first region is used (by default that's us-east-1) so the specified region is actually used.
  • Added Redshift instance types
  • Added some more logging to the script

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@codecov-io
Copy link

codecov-io commented Jan 15, 2019

Codecov Report

Merging #586 into master will increase coverage by 0.06%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #586      +/-   ##
==========================================
+ Coverage   84.82%   84.89%   +0.06%     
==========================================
  Files         113      113              
  Lines        6439     6453      +14     
  Branches     1504     1505       +1     
==========================================
+ Hits         5462     5478      +16     
+ Misses        629      627       -2     
  Partials      348      348
Impacted Files Coverage Δ
src/cfnlint/rules/parameters/AvailabilityZone.py 91.48% <ø> (ø) ⬆️
.../rules/resources/properties/PropertiesTemplated.py 100% <ø> (ø) ⬆️
...c/cfnlint/rules/resources/properties/Properties.py 76.7% <ø> (ø) ⬆️
src/cfnlint/rules/functions/GetAtt.py 87.71% <100%> (+0.68%) ⬆️
src/cfnlint/rules/outputs/Value.py 88.88% <100%> (+0.42%) ⬆️
...nlint/rules/resources/properties/ValueRefGetAtt.py 87.28% <100%> (-0.11%) ⬇️
...cfnlint/rules/resources/properties/AllowedValue.py 100% <100%> (ø) ⬆️
src/cfnlint/rules/resources/properties/Required.py 91.2% <100%> (+0.29%) ⬆️
src/cfnlint/__init__.py 86.83% <100%> (+0.05%) ⬆️
...nt/rules/functions/DynamicReferenceSecureString.py 97.29% <100%> (+0.11%) ⬆️
... and 4 more

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 12eb43b...944105d. Read the comment docs.

Copy link
Contributor

@kddejong kddejong left a comment

Choose a reason for hiding this comment

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

Looking good so far.

src/cfnlint/rules/parameters/AllowedValue.py Outdated Show resolved Hide resolved
Maybe rebuild into something "smarter"
@fatbasstard
Copy link
Contributor Author

Meh, messed up GitHub.. Damn

Instead of using a hardcoded region, all rules fetch the data from the Template now (which is defaulted to `us-east-1`).

In short term this means that if 1 region is specified in the linter, it actually uses that region
@fatbasstard fatbasstard changed the title [WIP] Parse pricing API to get allowed values Parse pricing API to get allowed values Jan 22, 2019
@kddejong
Copy link
Contributor

Sorry for the delay in reviewing this. I haven't had the time to go heads down on the initialize changes. There are a few scenarios related to that I want to run through and make sure we have them covered.

Copy link
Contributor

@kddejong kddejong left a comment

Choose a reason for hiding this comment

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

The addition of checking values has brought some new interesting view points to how we should handle regions in the rules. I'm not convinced this is the right solution yet but it is better than what it was.

@kddejong kddejong merged commit ee7e2fc into aws-cloudformation:master Jan 25, 2019
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.

3 participants