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

AWS: add ec2_transit_gateway_info module #54460

Merged
merged 9 commits into from
Apr 12, 2019

Conversation

BobBoldin
Copy link
Contributor

SUMMARY

Add new info module ec2_transit_gateway_info to compliment the ec2_transit_gateway module

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

ec2_transit_gateway_info

ADDITIONAL INFORMATION

Module will return information about AWS Transit Gateway objects. Used _info in name to conform with issue #54280

To pass integration tests will need permissions from network-policy.json added to CI environment.
'''
{
"Sid": "AllowTransitGatewayManagement",
"Effect": "Allow",
"Action": [
"ec2:CreateTransitGateway",
"ec2:DeleteTransitGateway",
"ec2:DescribeTransitGateways"
],
"Resource": "*"
}
'''

@BobBoldin
Copy link
Contributor Author

needs_ci_update
ready_for_review

@ansibot ansibot added affects_2.8 This issue/PR affects Ansible v2.8 aws cloud module This issue/PR relates to a module. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. needs_triage Needs a first human triage before being processed. new_module This PR includes a new module. new_plugin This PR includes a new plugin. support:community This issue/PR relates to code supported by the Ansible community. labels Mar 27, 2019
@BobBoldin
Copy link
Contributor Author

@mattclay As this is a basic info module, should I wait on perms, or change aliases to unsupported?

@mattclay
Copy link
Member

@BobBoldin I've updated CI permissions so this test will now pass. I'm also going to enable the existing ec2_transit_gateway integration tests in #54619.

To improve test performance and avoid unnecessarily creating and deleting transit gateways, please add the integration tests for this module to the existing ec2_transit_gateway tests. Add ec2_transit_gateway_info to the aliases file for that test so tests will run correctly.

@mattclay mattclay removed the needs_triage Needs a first human triage before being processed. label Mar 29, 2019
@mattclay
Copy link
Member

@BobBoldin I also had to mark the ec2_transit_gateway test unstable since it sometimes attempts to delete transit gateways when they are in the pending state. Is that something you can take a look at fixing? Once fixed the unstable alias can be removed from the test.

@mattclay mattclay added the ci_verified Changes made in this PR are causing tests to fail. label Mar 29, 2019
@BobBoldin
Copy link
Contributor Author

@mattclay to confirm,
Within this PR branch I should remove the ...targets/ec2_transit_gateway_info directory
take those tasks and incorporate them with the ec2_transit_gateway tasks
update the aliases with ec2_transit_gateway adding a line ec2_transit_gateway_info

Reviewed
https://app.shippable.com/github/ansible/ansible/runs/116823/104/console it appears the TGW tgw-03192e0bac40a297f was already existing in a Pending state before the test began.

Confirmed in https://app.shippable.com/github/ansible/ansible/runs/116823/105/console is the same tgw-03192e0bac40a297f

Is there way to make those 2 steps run serial? Otherwise I'll look to describe them dynamically so one doesn't detect the other.

@mattclay
Copy link
Member

@BobBoldin Correct, remove the ec2_transit_gateway_info target, incorporating those tasks into the ec2_transit_gateway target to make use of the transit gateway creation already occurring there. Adding ec2_transit_gateway_info to the aliases file will tell ansible-test that changes to that module should trigger that test. This is needed since the test can only be named after one module.

Tests run in parallel because they run on multiple Python versions. Tests from other PRs may also run in parallel. Also, transit gateways that are deleted will remain in the account for some time in the deleted state even after the test finishes. The tests will need to handle these situations gracefully to prevent intermittent test failures.

@ansibot ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Apr 1, 2019
@ansibot
Copy link
Contributor

ansibot commented Apr 1, 2019

@Constantin007 @Constantin07 @Deepakkothandan @Etherdaemon @Java1Guy @Madhura-CSI @MichaelBaydoun @Sodki @Zeekin @adq @akazakov @alachaum @amir343 @anryko @bekelchik @brandond @captainkerk @chenl87 @defunctio @dennisconrad @dkhenry @fiunchinho @fivethreeo @flowerysong @garethr @gobins @gunzy83 @gurumaia @hsingh @hyperized @iiibrad @infectsoldier @j-carl @jarv @Java1Guy @jimbydamonk @jmenga @joelthompson @jonhadfield @jonmer85 @joshsouza @jsdalton @jsmartin @kaczynskid @leedm777 @linuxdynasty @loia @lwade @michaeljs1990 @minichate @mjschultz @mmochan @nand0p @naslanidis @nathanwebsterdotme @nerzhul @nickball @orthanc @ozbillwang @piontas @pjodouin @prasadkatti @psykotox @ptux @pwnall @raags @rafaeldriutti @rickmendes @roadmapper @rrey @ryansb @ryansydnor @scicoin-project @scottanderson42 @sdubrul @shepdelacreme @silviud @slapula @steynovich @tastychutney @tgerla @timmahoney @tombamford @tomislacker @tsiganenok @viper233 @whiter @willricardo @wilvk @wimnat @yaakov-github @zacblazic @zbal @zimbatm

As a maintainer of a module in the same namespace this new module has been submitted to, your vote counts for shipits. Please review this module and add shipit if you would like to see it merged.

click here for bot help

@ansibot ansibot added community_review In order to be merged, this PR must follow the community review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Apr 1, 2019
@BobBoldin
Copy link
Contributor Author

ready_for_review

@@ -1,3 +1,4 @@
cloud/aws
shippable/aws/group2
unstable # sometimes tries to delete while in state pending
ec2_transit_gateway_info
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting, I didn't knew this can be done. I guess I have to look into that :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CI team suggested as it increases efficiency in tests. Otherwise was having to create new resources simply to get results for the _info

Copy link
Contributor

Choose a reason for hiding this comment

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

I've the same situation for other modules, that would improve global test performance somewhat.

Copy link
Contributor

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

shipit

Didn't test, but looks good and tests seem to pass.

@ansibot ansibot added shipit This PR is ready to be merged by Core and removed community_review In order to be merged, this PR must follow the community review workflow. labels Apr 9, 2019
@ansibot ansibot added community_review In order to be merged, this PR must follow the community review workflow. and removed shipit This PR is ready to be merged by Core labels Apr 9, 2019
@willthames
Copy link
Contributor

All tests pass for me

@willthames willthames merged commit 9ddde6b into ansible:devel Apr 12, 2019
@willthames
Copy link
Contributor

Merged, thanks @BobBoldin

@BobBoldin BobBoldin deleted the ec2_transit_gateway_info branch April 12, 2019 17:08
ndclt pushed a commit to ndclt/ansible that referenced this pull request Jun 13, 2019
* AWS: add ec2_transit_gateway_info module

* move info module test to main module and ensure unique description for parallel tests

* add type designators to module options in documentation

* assign results and return instead of exit.  Add elements directive available with ansible 2.8

* assign results and return instead of exit

* get() method superfluous for module.params

* correct return type in documentation for ASN and format the sample for Tag correctly

* added random uid to test description - removing unstable alias

* change random uuid to resource_prefix to improve source of object identification in testing
@ansible ansible locked and limited conversation to collaborators Jul 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.8 This issue/PR affects Ansible v2.8 aws cloud community_review In order to be merged, this PR must follow the community review workflow. module This issue/PR relates to a module. new_module This PR includes a new module. new_plugin This PR includes a new plugin. support:community This issue/PR relates to code supported by the Ansible community.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants