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.

Sign up for GitHub

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

Jump to bottom

ec2_vpc_nat_gateway: increase integration tests coverage #387

Merged
merged 2 commits into from
Feb 24, 2021
Merged

ec2_vpc_nat_gateway: increase integration tests coverage #387

merged 2 commits into from
Feb 24, 2021

Conversation

alinabuzachis
Copy link
Contributor

@alinabuzachis alinabuzachis commented Feb 2, 2021 •
edited by tremble
Loading

SUMMARY

Increase integration test coverage for ec2_vpc_nat_gateway and ec2_vpc_nat_gateway_info

COMPONENT NAME

ec2_vpc_nat_gateway
ec2_vpc_nat_gateway_info

ISSUE TYPE
  • Test Pull Request

@ansibullbot
Copy link

@alinabuzachis: Greetings! Thanks for taking the time to open this pullrequest. In order for the community to handle your pullrequest effectively, we need a bit more information.

Here are the items we could not find in your description:

  • issue type

Please set the description of this pullrequest with this template:
https://raw.githubusercontent.com/ansible/ansible/devel/.github/PULL_REQUEST_TEMPLATE.md

click here for bot help

@ansibullbot
Copy link

cc @Etherdaemon @jillr @jonhadfield @linuxdynasty @s-hertel @tremble @wimnat
click here for bot help

@ansibullbot ansibullbot added WIP Work in progress integration tests/integration module module needs_info This issue requires further information. Please answer any outstanding questions needs_template This issue/PR has an incomplete description. Please fill in the proposed template correctly needs_triage plugins plugin (any type) tests tests labels Feb 2, 2021
@ansibullbot ansibullbot removed the module module label Feb 3, 2021
@alinabuzachis alinabuzachis marked this pull request as ready for review February 11, 2021 18:37
@alinabuzachis alinabuzachis changed the title [WIP] ec2_vpc_nat_gateway: increase integration tests coverage ec2_vpc_nat_gateway: increase integration tests coverage Feb 11, 2021
@ansibullbot ansibullbot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR and removed WIP Work in progress labels Feb 11, 2021
tremble
tremble requested changes Feb 11, 2021
View reviewed changes
Copy link
Contributor

@tremble tremble left a comment

Choose a reason for hiding this comment

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

I'd like to see some form of tests against the shape of the data returned.

Where we search based on something like id, vpc or subnet we should probably check that we've been handed something that matches.

tests/integration/targets/ec2_vpc_nat_gateway/tasks/main.yml Outdated Show resolved Hide resolved
jillr
jillr reviewed Feb 11, 2021
View reviewed changes
Copy link
Collaborator

@jillr jillr left a comment

Choose a reason for hiding this comment

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

In addition to the tests you've added I see a few places we can add more assertions to the existing tests to help validate the module's behaviour.

Let's add an assertion to Search for internet gateway by VPC - no matches (line 42 task) that igw_info.internet_gateways has a length of zero, to confirm there really are no matches

We should also assert the shape of the return in other places where we create a gateway, like for the task at line 116 (assertion at line 125). That's the first place we create a gw so that's our first chance to check the results that the module returns are accurate.

Anywhere that a NAT gateway is searched for, the assertion should check the ID that was returned is what was expected.

For example at the task at line 150, "Get NAT gateway with specific filters (state and subnet)", the assertion should ensure the correct gateway ID is in the result.
And after line 288, "Fetch NAT gateway by ID (list)".

tests/integration/targets/ec2_vpc_nat_gateway/tasks/main.yml Outdated Show resolved Hide resolved
@alinabuzachis alinabuzachis force-pushed the ec2_vpc_nat_gateway branch 2 times, most recently from 52458ba to 577972f Compare February 16, 2021 17:21
jillr
jillr approved these changes Feb 22, 2021
View reviewed changes
Copy link
Collaborator

@jillr jillr left a comment

Choose a reason for hiding this comment

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

lgtm, anything you'd like to see @tremble?
Thanks for this work @alinabuzachis

tests/integration/targets/ec2_vpc_nat_gateway/tasks/main.yml Outdated Show resolved Hide resolved
tremble
tremble approved these changes Feb 23, 2021
View reviewed changes
Copy link
Contributor

@tremble tremble left a comment

Choose a reason for hiding this comment

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

Looks like a big improvement. Thanks for your work on this.

@ansibullbot ansibullbot removed needs_info This issue requires further information. Please answer any outstanding questions needs_template This issue/PR has an incomplete description. Please fill in the proposed template correctly labels Feb 23, 2021
@alinabuzachis
NAT gateway: increase integration tests coverage
b36fefa 
     * Add additional integration tests for ec2_vpc_nat_gateway_info and ec2_vpc_nat_gatewy modules
     * Add ec2_vpc_nat_gateway_info in aliases
     * Fix NAT gateway search

Signed-off-by: Alina Buzachis <[email protected]>
@alinabuzachis alinabuzachis force-pushed the ec2_vpc_nat_gateway branch 2 times, most recently from 77f6629 to 0854c1c Compare February 23, 2021 14:53
@jillr jillr merged commit fbfc351 into ansible-collections:main Feb 24, 2021
alinabuzachis added a commit to alinabuzachis/community.aws that referenced this pull request Jul 19, 2021
@alinabuzachis
ec2_vpc_nat_gateway: increase integration tests coverage (ansible-col…
2f152b5 
…lections#387)

* NAT gateway: increase integration tests coverage
     * Add additional integration tests for ec2_vpc_nat_gateway_info and ec2_vpc_nat_gatewy modules
     * Add ec2_vpc_nat_gateway_info in aliases
     * Fix NAT gateway search

Signed-off-by: Alina Buzachis <[email protected]>
alinabuzachis added a commit to alinabuzachis/community.aws that referenced this pull request Jul 19, 2021
@alinabuzachis
ec2_vpc_nat_gateway: increase integration tests coverage (ansible-col…
0ee2ccd 
…lections#387)

* NAT gateway: increase integration tests coverage
     * Add additional integration tests for ec2_vpc_nat_gateway_info and ec2_vpc_nat_gatewy modules
     * Add ec2_vpc_nat_gateway_info in aliases
     * Fix NAT gateway search

Signed-off-by: Alina Buzachis <[email protected]>
danielcotton pushed a commit to danielcotton/community.aws that referenced this pull request Nov 23, 2021
@alinabuzachis @danielcotton
ec2_vpc_nat_gateway: increase integration tests coverage (ansible-col…
8cf8771 
…lections#387)

* NAT gateway: increase integration tests coverage
     * Add additional integration tests for ec2_vpc_nat_gateway_info and ec2_vpc_nat_gatewy modules
     * Add ec2_vpc_nat_gateway_info in aliases
     * Fix NAT gateway search

Signed-off-by: Alina Buzachis <[email protected]>
alinabuzachis pushed a commit to alinabuzachis/community.aws that referenced this pull request May 25, 2022
@2d6
s3_bucket - Improve documentation of the policy parameter (ansible-co…
6c767b1 
…llections#387)

s3_bucket - improve documentation of policy parameter

SUMMARY
This pull requests improves the documentation of the policy parameter in the s3_bucket module. It documents how to ensure the absence of a policy.
Fixes ansible-collections#385
ISSUE TYPE

Docs Pull Request

COMPONENT NAME
s3_bucket

Reviewed-by: Jill R <None>
Reviewed-by: Moritz Wagner <None>
Reviewed-by: None <None>
Reviewed-by: Mark Chappell <None>
alinabuzachis added a commit to alinabuzachis/community.aws that referenced this pull request May 25, 2022
@alinabuzachis @jillr
ec2_vpc_nat_gateway: increase integration tests coverage (ansible-col…
d76dcdc 
…lections#387)

* NAT gateway: increase integration tests coverage
     * Add additional integration tests for ec2_vpc_nat_gateway_info and ec2_vpc_nat_gatewy modules
     * Add ec2_vpc_nat_gateway_info in aliases
     * Fix NAT gateway search

Signed-off-by: Alina Buzachis <[email protected]>

This commit was initially merged in https://github.com/ansible-collections/community.aws
See: ansible-collections@fbfc351
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jillr jillr jillr approved these changes

@tremble tremble tremble approved these changes

Assignees
No one assigned
Labels
integration tests/integration needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR needs_triage plugins plugin (any type) tests tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@alinabuzachis @ansibullbot @tremble @jillr