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_route_table: Don't fail if a route was already created. #359

Merged
merged 3 commits into from
Mar 24, 2021

Conversation

stefanhorning
Copy link
Contributor

SUMMARY

Fixes #357

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

ec2_vpc_route_table

ADDITIONAL INFORMATION

Catches error if route already exists. Not sure if all params get compared though for that exception to happen.

@ansibullbot
Copy link

@ansibullbot ansibullbot added bug This issue/PR relates to a bug has_issue module module needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR needs_triage plugins plugin (any type) labels Jan 14, 2021
@stefanhorning
Copy link
Contributor Author

stefanhorning commented Jan 15, 2021

Build fails with a request limit issue:

ClientError: An error occurred (RequestLimitExceeded) when calling the DescribeInternetGateways operation (reached max retries: 4): Request limit exceeded.

Wonder if my change did anything in this regard, or if we have to fix existing tests to pause between AWS requests (i.e. module executions)

@alinabuzachis
Copy link
Contributor

Hi @stefanhorning, thank you for your contribution. May you kindly add a changelog https://docs.ansible.com/ansible/latest/community/development_process.html#changelogs and rebase? In addition, it would also be good if you could add a functional test to make sure the change is working correctly. If you need any help please feel free to reach us on IRC #ansible-aws.

@ansibullbot ansibullbot added the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Mar 5, 2021
@stefanhorning stefanhorning force-pushed the vpc_route_tables_fix branch from fe73fd1 to d8d800e Compare March 9, 2021 10:25
@ansibullbot ansibullbot removed the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Mar 9, 2021
@tremble
Copy link
Contributor

tremble commented Mar 14, 2021

Hi @stefanhorning,

sorry for the delay in looking into this PR. The PR looks sensible enough.

To reduce the risk of someone accidentally introducing a regression, if possible please could you rebase and update the test suite to include an example of testing this specific case. I tried reproducing the issue and wasn't able to, so I'm not sure what the procedure to trigger the issue is:

  - name: CHECK MODE - add route to public route table
    ec2_vpc_route_table:
      vpc_id: "{{ vpc.vpc.id }}"
      tags:
        Public: "true"
        Name: "Public route table"
      routes:
      - dest: 0.0.0.0/0
        gateway_id: igw
    check_mode: True
    register: check_mode_results

  - name: assert a route would be added
    assert:
      that:
        - check_mode_results.changed

  - name: add a route to public route table
    ec2_vpc_route_table:
      vpc_id: "{{ vpc.vpc.id }}"
      tags:
        Public: "true"
        Name: "Public route table"
      routes:
      - dest: 0.0.0.0/0
        gateway_id: igw
    register: add_routes

  - name: assert route table contains new route
    assert:
      that:
        - add_routes.changed
        - add_routes.route_table.routes|length == 2

  - name: CHECK MODE - re-add route to public route table
    ec2_vpc_route_table:
      vpc_id: "{{ vpc.vpc.id }}"
      tags:
        Public: "true"
        Name: "Public route table"
      routes:
      - dest: 0.0.0.0/0
        gateway_id: igw 
    check_mode: True
    register: check_mode_results

  - name: assert a route would not be added
    assert:
      that:
        - check_mode_results is not changed

  - name: re-add a route to public route table
    ec2_vpc_route_table:
      vpc_id: "{{ vpc.vpc.id }}"
      tags:
        Public: "true"
        Name: "Public route table"
      routes:
      - dest: 0.0.0.0/0
        gateway_id: igw
    register: add_routes

  - name: assert route table contains route
    assert:
      that:
        - add_routes is not changed
        - add_routes.route_table.routes|length == 2

@ansibullbot ansibullbot added needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html and removed needs_triage labels Mar 14, 2021
@jillr
Copy link
Contributor

jillr commented Mar 19, 2021

Agreed - change looks good but a test would be helpful.
We'll be migrating this module to amazon.aws in early April so it would be good to get this merged before then if possible.

@stefanhorning
Copy link
Contributor Author

Rebased now. Will look into tests later.

@ansibullbot ansibullbot added community_review and removed 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 labels Mar 22, 2021
@goneri goneri requested a review from tremble March 24, 2021 12:59
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 have a vague memory of seeing the 'RouteAlreadyExists' in the integration tests before and that is seemed to be caused by the describe call misbehaving (which has been fixed by switching to a paginated lookup). In the interest of getting this fix in I'm going to get this merged and add a basic test in #496.

@tremble tremble merged commit 73d60e5 into ansible-collections:main Mar 24, 2021
alinabuzachis pushed a commit to alinabuzachis/community.aws that referenced this pull request Jul 19, 2021
alinabuzachis pushed a commit to alinabuzachis/community.aws that referenced this pull request Jul 19, 2021
danielcotton pushed a commit to danielcotton/community.aws that referenced this pull request Nov 23, 2021
alinabuzachis pushed a commit to alinabuzachis/community.aws that referenced this pull request May 25, 2022
``aws_s3`` copy to object

SUMMARY

add option to aws_s3 module to copy object existing on Amazon S3

Closes: ansible-collections#42
ISSUE TYPE


Feature Pull Request

COMPONENT NAME

aws_s3
ADDITIONAL INFORMATION



- name: copy from source to destination
  aws_s3:
      bucket: "{{ dest }}"
      mode: copy
      object: destination.txt
      copy_src:
        bucket: "{{ src }}"
        object: source.txt

Reviewed-by: Alina Buzachis <None>
Reviewed-by: Jill R <None>
Reviewed-by: Gonéri Le Bouder <goneri@lebouder.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue/PR relates to a bug community_review has_issue module module plugins plugin (any type)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ec2_vpc_route_table not idempotent - failing on adding already existing route
5 participants