Skip to content
This repository has been archived by the owner. It is now read-only.

Updated ec2_vpc_nat_gateway to use AWSRetry decorator #2721

Closed

Conversation

@linuxdynasty
Copy link
Contributor

@linuxdynasty linuxdynasty commented Aug 15, 2016

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

ec2_vpc_nat_gateway

ANSIBLE VERSION
ansible 2.1.1.0
SUMMARY

Updated ec2_vpc_nat_gateway to use the AWSRetry.backoff decorator. Also added fix, to make sure all output is returned in lower case. This module depends on the Pull Request AWSRetry.


This change is Reviewable

@linuxdynasty linuxdynasty force-pushed the linuxdynasty:ec2_vpc_nat_gateway_aws_retry branch from fc2b03e to 1eaebee Aug 15, 2016
@gregdek gregdek added the cloud label Aug 15, 2016
@gregdek
Copy link
Contributor

@gregdek gregdek commented Aug 15, 2016

Thanks again to @linuxdynasty. This module is going into community review.

We notice that you are one of the maintainers (@linuxdynasty) of this module, so when you think it's ready, please comment "shipit" and we will consider it for merging.

[This message brought to you by your friendly Ansibull-bot.]

@linuxdynasty linuxdynasty force-pushed the linuxdynasty:ec2_vpc_nat_gateway_aws_retry branch from 1eaebee to de8af54 Aug 18, 2016
@akurniawan
Copy link

@akurniawan akurniawan commented Sep 7, 2016

hi @linuxdynasty, since you're doing PR I want to file a suggestion for this module. In wait_for_status function within while wait_timeout > time.time(): loop, you put time.sleep(polling_increment_secs) within else section, is it better to put time.sleep outside try except? it would be a bit dangerous since it will do an api call repeatedly without sleeping if we got an exception. I'm filing this request because when I ran this module and waiting for nat_gateway to be created, suddenly I couldn't open my aws console saying the API call has reached its maximum limit. I'm not really sure whether this is the root cause, I will check it tomorrow morning

@ryansb
Copy link
Contributor

@ryansb ryansb commented Sep 9, 2016

@linuxdynasty are you intentionally deleting all those tests?

@linuxdynasty
Copy link
Contributor Author

@linuxdynasty linuxdynasty commented Sep 20, 2016

@ryansb I did delete the api integration tests from the PR.

@linuxdynasty linuxdynasty force-pushed the linuxdynasty:ec2_vpc_nat_gateway_aws_retry branch from de8af54 to cd4516e Sep 20, 2016
@linuxdynasty
Copy link
Contributor Author

@linuxdynasty linuxdynasty commented Sep 20, 2016

@akurniawan, This is AWS rate limiting the number of api requests that come in. Which this PR gets around by applying the AWSRetry decorator. To answer your question, when provisioning to AWS and you are creating x amount of resources, it is possible that you will receive that Rate Limiting error in the AWS console.

@linuxdynasty
Copy link
Contributor Author

@linuxdynasty linuxdynasty commented Sep 20, 2016

shipit

@gregdek
Copy link
Contributor

@gregdek gregdek commented Sep 20, 2016

Thanks @linuxdynasty. Since you are one of the maintainers (@linuxdynasty) of this module and you commented with "shipit", we are marking this PR for inclusion.

[This message brought to you by your friendly Ansibull-bot.]

@gregdek gregdek added shipit and removed community_review labels Sep 20, 2016
@bcoca bcoca added needs_rebase and removed shipit labels Nov 7, 2016
@gregdek
Copy link
Contributor

@gregdek gregdek commented Nov 8, 2016

Thanks @linuxdynasty for this PR. Unfortunately, it is not mergeable in its current state due to merge conflicts. Please rebase your PR. When you are done, please comment with text 'ready_for_review' and we will put this PR back into review.

For help on how to do this cleanly please see http://docs.ansible.com/ansible/community.html#contributing-code-features-or-bugfixes

[This message brought to you by your friendly Ansibull-bot.]

@gregdek
Copy link
Contributor

@gregdek gregdek commented Nov 23, 2016

@linuxdynasty A friendly reminder: this pull request has been marked as needing your action. If you still believe that this PR applies, and you intend to address the issues with this PR, just let us know in the PR itself and we will keep it open pending your changes. When you do address the issues, please respond with ready_for_review in your comment, so that we can notify the maintainer.

[This message brought to you by your friendly Ansibull-bot.]

@ryansb
Copy link
Contributor

@ryansb ryansb commented Dec 5, 2016

@linuxdynasty looks good to me, but the test changes need to be made to the ansible/ansible repo, since that file was moved in cf52467

@ansibot
Copy link

@ansibot ansibot commented Dec 6, 2016

This repository has been locked. All new issues and pull requests should be filed in https://github.com/ansible/ansible

Please read through the repomerge page in the dev guide. The guide contains links to tools which automatically move your issue or pull request to the ansible/ansible repo.

1 similar comment
@ansibot
Copy link

@ansibot ansibot commented Apr 11, 2017

This repository has been locked. All new issues and pull requests should be filed in https://github.com/ansible/ansible

Please read through the repomerge page in the dev guide. The guide contains links to tools which automatically move your issue or pull request to the ansible/ansible repo.

@ansibot ansibot closed this Apr 11, 2017
@sivel sivel removed the needs_revision label Apr 11, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.