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

ec2_eip: move to boto3, add name and tags #55190

Open
wants to merge 3 commits into
base: devel
from

Conversation

Projects
None yet
5 participants
@healem
Copy link
Contributor

commented Apr 11, 2019

SUMMARY
  • update to boto3(no longer dependent on boto)
  • add name and tags support
    • name can be used to specify the eip (uses the "Name" tag in AWS)
    • name and tags added to command output
  • change "in_vpc" to "ec2_classic" (with backwards compatibility)
    • This reverses the default logic to better align with AWS deprecation of EC2-Classic instances
  • fixed numerous issues with allocating and deleting eips
  • fixed the examples so they now work and align with current functionality
  • added significant integration testing
  • flattened and simplified much of the branching logic

Fixes:
#29871
#34422

Idempotency should be improved with this PR if "name" and/or "public_ip" are used for identification, but I don't feel comfortable saying it is fully fixed without more test coverage.

This closes #50539

ISSUE TYPE
  • Bugfix Pull Request
  • Feature Pull Request
COMPONENT NAME

ec2_eip

ADDITIONAL INFORMATION

Sanity checks run locally against python 2.7, 3.5, and 3.7 using the following command and varying the python version:
ansible-test sanity -v --docker --python 2.7 ec2_eip

Manually ran the integration test:
ansible-test integration ec2_eip --docker --python 3.7

An example of the new output (captured from the integration test):

ok: [testhost] => {
    "eip_result3": {
        "allocation_id": "eipalloc-0a550674664cbc5ab",
        "changed": true,
        "failed": false,
        "name": "myEIP1",
        "public_ip": "54.92.137.242",
        "resource_actions": [
            "ec2:AssociateAddress",
            "ec2:DescribeAddresses"
        ],
        "tags": {
            "Name": "myEIP1",
            "example": "test tag"
        },
    }
}

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Apr 11, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Apr 11, 2019

@healem, just so you are aware we have a dedicated Working Group for aws.
You can find other people interested in this in #ansible-aws on Freenode IRC
For more information about communities, meetings and agendas see https://github.com/ansible/community

click here for bot help

@healem

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2019

Tests 102, 103 failed with:
"msg": "You are not authorized to perform this operation."

That looks to be a CI issue, similar to #54950? Not sure there is much I can do about this.

Test 1 failed with:
19:04 ERROR: lib/ansible/modules/cloud/amazon/ec2_eip.py:0:0: E309 version_added for new option (ec2_classic) should be '1.4'. Currently '2.8' (75%)

Since I just introduced the ec2_classic option, it seems like setting the version_added field to 2.8 is the correct thing to do. However, it does alias an older option (in_vpc) to preserve backwards compatibility. Does that mean I should set version_added to 1,4?

(edited: other errors fixed by removing manage_tags from ec2.py)

@ansibot ansibot added needs_revision and removed core_review labels Apr 11, 2019

@jeking3
Copy link
Contributor

left a comment

A couple questions. Nice integration test!

Show resolved Hide resolved lib/ansible/module_utils/ec2.py Outdated
Show resolved Hide resolved lib/ansible/modules/cloud/amazon/ec2_eip.py
Show resolved Hide resolved lib/ansible/modules/cloud/amazon/ec2_eip.py
@s-hertel
Copy link
Contributor

left a comment

The permissions failures in CI will need to be fixed by someone on the Ansible team. There is currently a queue.

Have you seen #50539? Please review it! It's more efficient to join efforts. It needs community reviews. That pull request was based off the initial work to port ec2_eip and picked up by someone new when the original contributor no longer had time for it. This is a really handy tool I use to find pull requests modifying the same file https://ansible.sivel.net/pr/byfile.html.

Show resolved Hide resolved lib/ansible/module_utils/ec2.py Outdated
@healem

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2019

I had not seen PR #50539. Thanks for pointing it out - I'll review it. Also, thank you for the pointer to the tool. That looks really useful.

@ansibot ansibot removed the needs_triage label Apr 12, 2019

@healem healem referenced this pull request Apr 12, 2019

Closed

Migrate EC2 EIP to boto3 #50539

ec2_eip: move to boto3, add name and tags support, change in_vpc to e…
…c2_classic (with backwards compatibility) and reverse default logic to better align with AWS deprecation of EC2-Classic instances, fixed numerous issues with allocating and deleting eips

@healem healem force-pushed the healem:eip_work branch from 0787b78 to a0473f5 Apr 12, 2019

@ansibot ansibot removed the support:core label Apr 12, 2019

@jeking3

This comment has been minimized.

Copy link
Contributor

commented Apr 13, 2019

I can confirm this provides idempotent behavior leveraging the name.

@healem

This comment has been minimized.

Copy link
Contributor Author

commented Apr 19, 2019

The only thing left for this is to get the CI permissions fixed for the integration test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.