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

Improve doc to show support for IPv6 CIDR block #634

Conversation

sebastien-rosset
Copy link
Contributor

@sebastien-rosset sebastien-rosset commented Jan 24, 2022

SUMMARY
  1. Improve doc to show IPv6 CIDR blocks are supported.
  2. Add example with IPv6 CIDR block.
  3. Add missing attribute to return values.
  4. Remove duplicate assertions in integration tests.
  5. Add tests for IPv6 subnets in integration tests.
ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

ec2_vpc_route_table

ADDITIONAL INFORMATION
  1. While testing IPv6 in this module, I discovered the ipsubnet filter does not work if the prefix length has a fairly high value such as /120.
    1. I was running an integration test with a /120 subnet in this PR, but the ipsubnet call never returns and uses 100% CPU. I changed the value to a /64 subnet so this PR can pass.
    2. The ipsubnet issue needs to be fixed in ansible.netcommon and the netaddr package. I've created a unit test to reproduce the problem: Add unit tests for ipsubnet filter and IPv6  ansible.netcommon#362
  2. The integration tests in this PR depend on vpc_net check mode, IPV6 CIDR assoc/disassoc #631 for the VPC configuration.

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.

Thanks for taking the time to try and improve the docs and tests.

It looks like some random pasting errors crept in.

plugins/modules/ec2_vpc_route_table.py Outdated Show resolved Hide resolved
plugins/modules/ec2_vpc_route_table.py Outdated Show resolved Hide resolved
@alinabuzachis
Copy link
Contributor

recheck

@sebastien-rosset
Copy link
Contributor Author

@alinabuzachis , FYI this PR will fail integration tests until #631 is merged.

- cidr: 10.228.231.0/24
assign_instances_ipv6: true
# Carve first /64 subnet of the Amazon-provided CIDR for the VPC.
ipv6_cidr: "{{ vpc_ipv6_cidr_block | ansible.netcommon.ipsubnet(64, 1) }}"
Copy link
Contributor Author

@sebastien-rosset sebastien-rosset Jan 24, 2022

Choose a reason for hiding this comment

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

Initially I was testing with a /120 prefix length, not a /64.
However, that caused the test to be stuck, and I uncovered ansible-collections/ansible.utils#132 and netaddr/netaddr#241

@ansibullbot
Copy link

@ansibullbot ansibullbot added bug This issue/PR relates to a bug community_review integration tests/integration module module needs_triage new_contributor Help guide this first time contributor plugins plugin (any type) tests tests labels Jan 27, 2022
@jillr jillr removed the needs_triage label Feb 8, 2022
@alinabuzachis alinabuzachis added the backport-3 PR should be backported to the stable-3 branch label Mar 21, 2022
Copy link
Contributor

@alinabuzachis alinabuzachis left a comment

Choose a reason for hiding this comment

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

@sebastien-rosset Thank you. LGTM!

@ansibullbot ansibullbot removed the new_contributor Help guide this first time contributor label Mar 21, 2022
@ansibullbot
Copy link

@sebastien-rosset this PR contains the following merge commits:

Please rebase your branch to remove these commits.

click here for bot help

@ansibullbot ansibullbot added merge_commit This PR contains at least one merge commit. Please resolve! needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html and removed community_review labels Mar 21, 2022
sebastien-rosset and others added 4 commits March 21, 2022 14:56
Co-authored-by: Mark Chappell <mchappel@redhat.com>
Co-authored-by: Mark Chappell <mchappel@redhat.com>
Co-authored-by: Mark Chappell <mchappel@redhat.com>
@ansibullbot ansibullbot added community_review and removed needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html labels Mar 21, 2022
@softwarefactory-project-zuul
Copy link
Contributor

Build failed.

@alinabuzachis
Copy link
Contributor

recheck

@alinabuzachis alinabuzachis added the backport-2 PR should be backported to the stable-2 branch label Mar 22, 2022
@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@alinabuzachis alinabuzachis added mergeit Merge the PR (SoftwareFactory) and removed mergeit Merge the PR (SoftwareFactory) labels Mar 24, 2022
@jillr
Copy link
Collaborator

jillr commented Mar 25, 2022

regate

1 similar comment
@alinabuzachis
Copy link
Contributor

regate

@alinabuzachis alinabuzachis dismissed tremble’s stale review March 28, 2022 15:02

It seems it stucks gating this PR.

@alinabuzachis
Copy link
Contributor

regate

@softwarefactory-project-zuul
Copy link
Contributor

Build failed (gate pipeline). For information on how to proceed, see
http://docs.openstack.org/infra/manual/developers.html#automated-testing

@alinabuzachis
Copy link
Contributor

regate

@softwarefactory-project-zuul
Copy link
Contributor

Build failed (gate pipeline). For information on how to proceed, see
http://docs.openstack.org/infra/manual/developers.html#automated-testing

@alinabuzachis
Copy link
Contributor

regate

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded (gate pipeline).

@softwarefactory-project-zuul softwarefactory-project-zuul bot merged commit c06d77c into ansible-collections:main Mar 29, 2022
@patchback
Copy link

patchback bot commented Mar 29, 2022

Backport to stable-2: 💔 cherry-picking failed — conflicts found

❌ Failed to cleanly apply c06d77c on top of patchback/backports/stable-2/c06d77c94a70a767566464b299410a27ea55f3e1/pr-634

Backporting merged PR #634 into main

  1. Ensure you have a local repo clone of your fork. Unless you cloned it
    from the upstream, this would be your origin remote.
  2. Make sure you have an upstream repo added as a remote too. In these
    instructions you'll refer to it by the name upstream. If you don't
    have it, here's how you can add it:
    $ git remote add upstream https://github.com/ansible-collections/amazon.aws.git
  3. Ensure you have the latest copy of upstream and prepare a branch
    that will hold the backported code:
    $ git fetch upstream
    $ git checkout -b patchback/backports/stable-2/c06d77c94a70a767566464b299410a27ea55f3e1/pr-634 upstream/stable-2
  4. Now, cherry-pick PR Improve doc to show support for IPv6 CIDR block #634 contents into that branch:
    $ git cherry-pick -x c06d77c94a70a767566464b299410a27ea55f3e1
    If it'll yell at you with something like fatal: Commit c06d77c94a70a767566464b299410a27ea55f3e1 is a merge but no -m option was given., add -m 1 as follows intead:
    $ git cherry-pick -m1 -x c06d77c94a70a767566464b299410a27ea55f3e1
  5. At this point, you'll probably encounter some merge conflicts. You must
    resolve them in to preserve the patch from PR Improve doc to show support for IPv6 CIDR block #634 as close to the
    original as possible.
  6. Push this branch to your fork on GitHub:
    $ git push origin patchback/backports/stable-2/c06d77c94a70a767566464b299410a27ea55f3e1/pr-634
  7. Create a PR, ensure that the CI is green. If it's not — update it so that
    the tests and any other checks pass. This is it!
    Now relax and wait for the maintainers to process your pull request
    when they have some cycles to do reviews. Don't worry — they'll tell you if
    any improvements are necessary when the time comes!

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

@patchback
Copy link

patchback bot commented Mar 29, 2022

Backport to stable-3: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-3/c06d77c94a70a767566464b299410a27ea55f3e1/pr-634

Backported as #751

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

patchback bot pushed a commit that referenced this pull request Mar 29, 2022
Improve doc to show support for IPv6 CIDR block

SUMMARY

Improve doc to show IPv6 CIDR blocks are supported.
Add example with IPv6 CIDR block.
Add missing attribute to return values.
Remove duplicate assertions in integration tests.
Add tests for IPv6 subnets in integration tests.

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME

ec2_vpc_route_table
ADDITIONAL INFORMATION

While testing IPv6 in this module, I discovered the ipsubnet filter does not work if the prefix length has a fairly high value such as /120.

I was running an integration test with a /120 subnet in this PR, but the ipsubnet call never returns and uses 100% CPU. I changed the value to a /64 subnet so this PR can pass.
The ipsubnet issue needs to be fixed in ansible.netcommon and the netaddr package. I've created a unit test to reproduce the problem: ansible-collections/ansible.netcommon#362

The integration tests in this PR depend on #631 for the VPC configuration.

Reviewed-by: Mark Chappell <None>
Reviewed-by: Sebastien Rosset <None>
Reviewed-by: Alina Buzachis <None>
Reviewed-by: Jill R <None>
(cherry picked from commit c06d77c)
softwarefactory-project-zuul bot pushed a commit that referenced this pull request Mar 29, 2022
[PR #634/c06d77c9 backport][stable-3] Improve doc to show support for IPv6 CIDR block

This is a backport of PR #634 as merged into main (c06d77c).
SUMMARY


Improve doc to show IPv6 CIDR blocks are supported.
Add example with IPv6 CIDR block.
Add missing attribute to return values.
Remove duplicate assertions in integration tests.
Add tests for IPv6 subnets in integration tests.


ISSUE TYPE


Bugfix Pull Request

COMPONENT NAME

ec2_vpc_route_table
ADDITIONAL INFORMATION



While testing IPv6 in this module, I discovered the ipsubnet filter does not work if the prefix length has a fairly high value such as /120.

I was running an integration test with a /120 subnet in this PR, but the ipsubnet call never returns and uses 100% CPU. I changed the value to a /64 subnet so this PR can pass.
The ipsubnet issue needs to be fixed in ansible.netcommon and the netaddr package. I've created a unit test to reproduce the problem: ansible-collections/ansible.netcommon#362


The integration tests in this PR depend on #631 for the VPC configuration.
alinabuzachis pushed a commit to alinabuzachis/amazon.aws that referenced this pull request Mar 31, 2022
) (ansible-collections#751)

[PR ansible-collections#634/c06d77c9 backport][stable-3] Improve doc to show support for IPv6 CIDR block

This is a backport of PR ansible-collections#634 as merged into main (c06d77c).
SUMMARY


Improve doc to show IPv6 CIDR blocks are supported.
Add example with IPv6 CIDR block.
Add missing attribute to return values.
Remove duplicate assertions in integration tests.
Add tests for IPv6 subnets in integration tests.


ISSUE TYPE


Bugfix Pull Request

COMPONENT NAME

ec2_vpc_route_table
ADDITIONAL INFORMATION



While testing IPv6 in this module, I discovered the ipsubnet filter does not work if the prefix length has a fairly high value such as /120.

I was running an integration test with a /120 subnet in this PR, but the ipsubnet call never returns and uses 100% CPU. I changed the value to a /64 subnet so this PR can pass.
The ipsubnet issue needs to be fixed in ansible.netcommon and the netaddr package. I've created a unit test to reproduce the problem: ansible-collections/ansible.netcommon#362


The integration tests in this PR depend on ansible-collections#631 for the VPC configuration.
abikouo pushed a commit to abikouo/amazon.aws that referenced this pull request Oct 24, 2023
…new-tiny_prefix-variable_5260

tests: use the new tiny_prefix variable

SUMMARY
The new tiny_prefix variable has recently been introduced. It's
a 12 characters long string that is reused for the whole job.
ISSUE TYPE


Bugfix Pull Request

COMPONENT NAME
tests

Reviewed-by: Dmytro Vorotyntsev <None>
Reviewed-by: Mark Chappell <None>
Reviewed-by: None <None>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-2 PR should be backported to the stable-2 branch backport-3 PR should be backported to the stable-3 branch bug This issue/PR relates to a bug community_review integration tests/integration mergeit Merge the PR (SoftwareFactory) module module plugins plugin (any type) tests tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants