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

Raise ValueError for Outputs and Mappings - Fix Issue #732 #733

Merged
merged 3 commits into from
Jun 6, 2017
Merged

Raise ValueError for Outputs and Mappings - Fix Issue #732 #733

merged 3 commits into from
Jun 6, 2017

Conversation

apmclean
Copy link
Contributor

@apmclean apmclean commented Jun 6, 2017

Raises ValueError when CloudFormation template limits are reached for Outputs or Mappings.

Test cases included and passing.

Thanks!

Copy link
Member

@markpeek markpeek left a comment

Choose a reason for hiding this comment

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

Nice change and thanks for adding tests. For both outputs and mappings tests you should use the MAX_ values you defined in the change rather than hardcoding. For example, use range(0, MAX_OUTPUTS). For the ValueError case just use any name/value rather than needing it be MAX_OUTPUTS+1. Also, running pycodestyle and pyflakes locally will help fix some of the test errors.

@apmclean
Copy link
Contributor Author

apmclean commented Jun 6, 2017

Ok let me tune the tests up. I followed the existing test case paradigm for parameters and resources which have the maximums hard-coded for their ValueError. Do you want me to include a tune-up for those in the pull request too?

@markpeek
Copy link
Member

markpeek commented Jun 6, 2017

Ah, got it. Guess we didn't catch it before. Yes, if you're so inclined please tune-up the other ones as well. Thanks!

@apmclean
Copy link
Contributor Author

apmclean commented Jun 6, 2017

Would it make sense to expose the template limit maximums as attributes of the template class? Then the test cases could determine them programmatically.

Within the Template class init something like:

self.limits={
    'outputs': 60,
    'resources': 200,
    'parameters': 100,
    'mappings': 100
}

Then any future changes will just require changing the class versus the class + test cases.

@markpeek
Copy link
Member

markpeek commented Jun 6, 2017

Interesting idea. But I'm not sure if those would be used except for the test cases. Probably best not to clutter the template class with it at this time.

@apmclean
Copy link
Contributor Author

apmclean commented Jun 6, 2017

Understood. Cleaned up the test case and passed flakes and codestyle.

Thanks again for a great tool! I'll try to contribute in more meaningful ways in the future.

@markpeek markpeek merged commit 39ab2e7 into cloudtools:master Jun 6, 2017
@markpeek
Copy link
Member

markpeek commented Jun 6, 2017

Excellent. Thank you for the contribution and fixing those additional tests.

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.

2 participants