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

Fix calculation of VPC Network Interfaces limit #394

Merged
merged 2 commits into from
Mar 1, 2019
Merged

Fix calculation of VPC Network Interfaces limit #394

merged 2 commits into from
Mar 1, 2019

Conversation

TimGebert
Copy link
Contributor

Summary

Fixes calculation for VPC Network Interface limit.

Contributor License Agreement

By submitting this work for inclusion in awslimitchecker, I agree to the following terms:

  • The contribution included in this request (and any subsequent revisions or versions of it)
    is being made under the same license as the awslimitchecker project (the Affero GPL v3,
    or any subsequent version of that license if adopted by awslimitchecker).
  • My contribution may perpetually be included in and distributed with awslimitchecker; submitting
    this pull request grants a perpetual, global, unlimited license for it to be used and distributed
    under the terms of awslimitchecker's license.
  • I have the legal power and rights to agree to these terms.

@bergkampsliew
Copy link
Contributor

looks like unit test needs fixing

@jantman
Copy link
Owner

jantman commented Feb 28, 2019

@TimGebert

Thanks for the contribution! I wasn't aware of that bug in the NI limit calculation, and nobody else has noticed this yet either! It appears this was introduced in #379 and released in 6.1.0 about a month ago.

I'm going to be rather busy the next few days, so I might be able to get this fixed up and released while I'm working on other things, or else it might not be released until the weekend or early next week.

Yes, the unit test is definitely causing a build failure, because they're testing the previous logic. It should only take 3 changes to fix them. The unit test in question has a sample API response with "400" as the max-instances limit returned by the EC2 API, and expects the Network interfaces per Region limit to also be 400. Since it appears that the limit should really be multiplied by five but that was never done in the code, we'll want to expect the resulting Network interfaces per Region limit to be 2000 instead of 400... so the unit tests should be fixed by just changing the 400 on lines 385 and 386 of tests/services/test_vpc.py to 2000.

Thanks,
Jason

PS - Tim, this bug happened to be pretty obvious when looking at the changes in this PR. In the future, though, it would be helpful if you provided a more detailed overview of the problem for PRs that don't already have a matching issue containing all of the details requested in the issue template.

( cc @nadlerjessie on this PR, as I assume her employer is also using this code )

@codecov-io
Copy link

codecov-io commented Feb 28, 2019

Codecov Report

Merging #394 into develop will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           develop   #394   +/-   ##
======================================
  Coverage      100%   100%           
======================================
  Files           30     30           
  Lines         2274   2274           
  Branches       338    338           
======================================
  Hits          2274   2274
Impacted Files Coverage Δ
awslimitchecker/services/vpc.py 100% <100%> (ø) ⬆️

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 77414e6...47c2a06. Read the comment docs.

@jantman
Copy link
Owner

jantman commented Mar 1, 2019

I'm going to merge this now and try to get a release out today. I'll update when it's live on PyPI.

Thanks so much for the contribution, @TimGebert !!

@jantman jantman merged commit b0e28d4 into jantman:develop Mar 1, 2019
@jantman
Copy link
Owner

jantman commented Mar 1, 2019

This has been released as 6.1.4 and is live on PyPI.

Thank you so much!

@TimGebert
Copy link
Contributor Author

Thanks @jantman for making it easy to get this in so easily, and constantly supporting the repo :)

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.

4 participants