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_vpc_peer update allowing absent/present to perform all actions #37716

Closed
wants to merge 1 commit into from
Closed

ec2_vpc_peer update allowing absent/present to perform all actions #37716

wants to merge 1 commit into from

Conversation

andynelson
Copy link
Contributor

SUMMARY

This update:

  1. makes 'present' perform both create and accept tasks
  2. makes 'absent' perform both remove and reject tasks

In addition, on reject/removal tasks, the removed peering id is now returned

accept/reject are kept as synonyms for present/absent

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

ec2_vpc_peer.py

ANSIBLE VERSION
ansible 2.4.3.0
  config file = None
  configured module search path = [u'/Users/anelson/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /Users/anelson/virtualenv/Ansible-2.4/lib/python2.7/site-packages/ansible
  executable location = /Users/anelson/virtualenv/Ansible-2.4/bin/ansible
  python version = 2.7.10 (default, Feb  7 2017, 00:08:15) [GCC 4.2.1 Compatible Apple LLVM 8.0.0 (clang-800.0.34)]
ADDITIONAL INFORMATION

When running peering between multiple VPCs that may or may not be in the same inventory, running an explicit sequence of 'present' > 'accept' is no longer necessary - just running with 'present' against both inventories in any order will result in a completed peering.

The same is true for removal - state: absent

@andynelson andynelson changed the title update allowing absent/present to perform all actions ec2_vpc_peer update allowing absent/present to perform all actions Mar 21, 2018
@ansibot
Copy link
Contributor

ansibot commented Mar 21, 2018

@ansibot ansibot added aws cloud committer_review In order to be merged, this PR must follow the certified review workflow. feature This issue/PR relates to a feature request. module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. new_contributor This PR is the first contribution by a new community member. support:certified This issue/PR relates to certified code. labels Mar 21, 2018
@ansibot
Copy link
Contributor

ansibot commented Mar 21, 2018

The test ansible-test sanity --test ansible-doc --python 2.6 [explain] failed with 1 error:

lib/ansible/modules/cloud/amazon/ec2_vpc_peer.py:0:0: has a documentation error formatting or is missing documentation.

The test ansible-test sanity --test ansible-doc --python 2.7 [explain] failed with 1 error:

lib/ansible/modules/cloud/amazon/ec2_vpc_peer.py:0:0: has a documentation error formatting or is missing documentation.

The test ansible-test sanity --test ansible-doc --python 3.5 [explain] failed with 1 error:

lib/ansible/modules/cloud/amazon/ec2_vpc_peer.py:0:0: has a documentation error formatting or is missing documentation.

The test ansible-test sanity --test ansible-doc --python 3.6 [explain] failed with 1 error:

lib/ansible/modules/cloud/amazon/ec2_vpc_peer.py:0:0: has a documentation error formatting or is missing documentation.

The test ansible-test sanity --test ansible-doc --python 3.7 [explain] failed with 1 error:

lib/ansible/modules/cloud/amazon/ec2_vpc_peer.py:0:0: has a documentation error formatting or is missing documentation.

The test ansible-test sanity --test pep8 [explain] failed with 5 errors:

lib/ansible/modules/cloud/amazon/ec2_vpc_peer.py:316:44: E231 missing whitespace after ','
lib/ansible/modules/cloud/amazon/ec2_vpc_peer.py:328:12: E225 missing whitespace around operator
lib/ansible/modules/cloud/amazon/ec2_vpc_peer.py:354:32: E225 missing whitespace around operator
lib/ansible/modules/cloud/amazon/ec2_vpc_peer.py:357:32: E225 missing whitespace around operator
lib/ansible/modules/cloud/amazon/ec2_vpc_peer.py:360:32: E225 missing whitespace around operator

The test ansible-test sanity --test validate-modules [explain] failed with 5 errors:

lib/ansible/modules/cloud/amazon/ec2_vpc_peer.py:0:0: E324 Value for "default" from the argument_spec ('present') for "state" does not match the documentation (None)
lib/ansible/modules/cloud/amazon/ec2_vpc_peer.py:0:0: E324 Value for "default" from the argument_spec (True) for "validate_certs" does not match the documentation (None)
lib/ansible/modules/cloud/amazon/ec2_vpc_peer.py:0:0: E325 argument_spec for "validate_certs" defines type="bool" but documentation does not
lib/ansible/modules/cloud/amazon/ec2_vpc_peer.py:0:0: E326 Value for "choices" from the argument_spec (['present', 'absent', 'accept', 'reject']) for "state" does not match the documentation ([])
lib/ansible/modules/cloud/amazon/ec2_vpc_peer.py:50:19: E302 DOCUMENTATION is not valid YAML

The test ansible-test sanity --test yamllint [explain] failed with 1 error:

lib/ansible/modules/cloud/amazon/ec2_vpc_peer.py:50:19: error DOCUMENTATION: syntax error: expected <block end>, but found '<scalar>'

click here for bot help

@ansibot ansibot added ci_verified Changes made in this PR are causing tests to fail. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed committer_review In order to be merged, this PR must follow the certified review workflow. ci_verified Changes made in this PR are causing tests to fail. labels Mar 21, 2018
@ansibot
Copy link
Contributor

ansibot commented Mar 21, 2018

The test ansible-test sanity --test pep8 [explain] failed with 1 error:

lib/ansible/modules/cloud/amazon/ec2_vpc_peer.py:49:161: E501 line too long (222 > 160 characters)

click here for bot help

@ansibot ansibot added the ci_verified Changes made in this PR are causing tests to fail. label Mar 21, 2018
@ansibot ansibot added committer_review In order to be merged, this PR must follow the certified review workflow. and removed ci_verified Changes made in this PR are causing tests to fail. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Mar 21, 2018
@samdoran samdoran removed the needs_triage Needs a first human triage before being processed. label Mar 22, 2018
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Mar 30, 2018
@ryansb ryansb requested a review from s-hertel May 3, 2018 12:27
@ansibot ansibot added the affects_2.6 This issue/PR affects Ansible v2.6 label May 19, 2018
Copy link
Contributor

@s-hertel s-hertel left a comment

Choose a reason for hiding this comment

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

From a cursory manual testing this looks good. A couple things: every API call should handle ClientError and BotoCoreError. If you update the module to use AnsibleAWSModule that will simplify the exception handling so you can just call module.fail_json_aws(e) and it will add the traceback and e.response if there is one. https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/cloud/amazon/GUIDELINES.md#using-fail_json_aws
Secondly, since this module is stableinterface there really needs to be integration tests to make sure backwards compatibility is not broken. Let me know if you need help putting that together (ec2_group as an example: https://github.com/ansible/ansible/tree/devel/test/integration/targets/ec2_group).

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed committer_review In order to be merged, this PR must follow the certified review workflow. labels Jul 13, 2018
@ansibot ansibot removed the new_contributor This PR is the first contribution by a new community member. label Jul 21, 2018
@ansibot ansibot removed the support:certified This issue/PR relates to certified code. label Sep 15, 2018
@ansibot ansibot added the support:core This issue/PR relates to code supported by the Ansible Engineering Team. label Sep 15, 2018
@ansibot ansibot added needs_maintainer Ansibot is unable to identify maintainers for this PR. (Check `author` in docs or BOTMETA.yml) support:certified This issue/PR relates to certified code. support:community This issue/PR relates to code supported by the Ansible community. and removed support:core This issue/PR relates to code supported by the Ansible Engineering Team. support:certified This issue/PR relates to certified code. labels Oct 9, 2018
@ansibot ansibot removed the needs_maintainer Ansibot is unable to identify maintainers for this PR. (Check `author` in docs or BOTMETA.yml) label Nov 10, 2018
@ansibot ansibot added the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Dec 4, 2018
@gundalow
Copy link
Contributor

@andynelson
Thank you for your PR.
As part of reviewing the backlog of PRs we are looking at PRs older PRs that haven't been updated in a while

Given that:

  • PR hasn't been updated since March 2018
  • Updates required in July 2018
  • This is a feature rather than a bug fix

Therefore I'm going to close this.

If you or anyone else wants to continue with this work then please do feel free to create a fresh PR and @mention the previous reviewers in here.

@gundalow gundalow closed this Sep 19, 2019
@gundalow gundalow added the pr_day Has been reviewed during a PR review Day label Sep 19, 2019
@ansible ansible locked and limited conversation to collaborators Oct 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.6 This issue/PR affects Ansible v2.6 aws cloud feature This issue/PR relates to a feature request. module This issue/PR relates to a module. needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. pr_day Has been reviewed during a PR review Day stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. 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