ec2_eni: Fix idempotency when security_groups is specified by ichekaldin · Pull Request #337 · ansible-collections/amazon.aws

ichekaldin · 2021-04-23T02:46:59Z

SUMMARY

If security_groups attribute is specified - module always returns Changed = True.

Fixes #255.

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME

ec2_eni

ADDITIONAL INFORMATION

Example:

- amazon.aws.ec2_eni:
    name: my-eni
    security_groups:
      - my-sg
    subnet_id: subnet-123456

Will always return Changed = True.

If `security_groups` attribute is specified - module always returns `Changed` = `True`. Example: ``` - amazon.aws.ec2_eni: name: my-eni security_groups: - my-sg subnet_id: subnet-123456 ``` Will always return `Changed` = `True`. This PR fixes that.

ansibullbot · 2021-04-23T02:51:48Z

cc @healem @jillr @s-hertel @tremble @wimnat
click here for bot help

ichekaldin · 2021-04-23T03:50:48Z

Strange. Integration tests fail with the following error:

ERROR! couldn't resolve module/action 'ec2_instance'. This often indicates a misspelling, missing collection, or incorrect module path.

The error appears to be in '/home/zuul/.ansible/collections/ansible_collections/amazon/aws/tests/integration/targets/ec2_eni/tasks/main.yaml': line 71, column 5, but may be elsewhere in the file depending on the exact syntax problem.

The line in question is:

  - name: Create 2 instances to test attaching and detaching network interfaces
    ec2_instance:

It has nothing to do with the changes I made. Has this integration test worked previously?

goneri · 2021-04-23T12:55:09Z

recheck

goneri · 2021-04-23T12:58:18Z

recheck

…ust ipaddr()

ichekaldin · 2021-04-25T19:37:36Z

The test fails with the following error:

TASK [ec2_eni : Assert that ec2_eni_info returns all the values we expect] *****
task path: /home/zuul/.ansible/collections/ansible_collections/amazon/aws/tests/integration/targets/ec2_eni/tasks/test_eni_basic_creation.yaml:24
[WARNING]: an unexpected error occurred during Jinja2 environment setup: unable
to locate collection ansible.netcommon
exception during Jinja2 environment setup: Traceback (most recent call last):
  File "/tmp/ansible-test-bntq2_0j/ansible/utils/collection_loader/_collection_finder.py", line 1058, in _get_collection_metadata
    collection_pkg = import_module('ansible_collections.' + collection_name)
  File "/usr/lib64/python3.6/importlib/__init__.py", line 126, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
  File "<frozen importlib._bootstrap>", line 994, in _gcd_import
  File "<frozen importlib._bootstrap>", line 971, in _find_and_load
  File "<frozen importlib._bootstrap>", line 953, in _find_and_load_unlocked
ModuleNotFoundError: No module named 'ansible_collections.ansible.netcommon'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/tmp/ansible-test-bntq2_0j/ansible/template/__init__.py", line 485, in __getitem__
    ts = _get_collection_metadata(acr.collection)
  File "/tmp/ansible-test-bntq2_0j/ansible/utils/collection_loader/_collection_finder.py", line 1060, in _get_collection_metadata
    raise ValueError('unable to locate collection {0}'.format(collection_name))
ValueError: unable to locate collection ansible.netcommon
fatal: [testhost]: FAILED! => {
    "msg": "The conditional check '_interface_0.private_ip_address | ansible.netcommon.ipaddr' failed. The error was: template error while templating string: unable to locate collection ansible.netcommon. String: {% if _interface_0.private_ip_address | ansible.netcommon.ipaddr %} True {% else %} False {% endif %}"
}

I think I hit a wall in trying to fix it.

I'm inclined to remove lines in 60 and 155 in tests/integration/targets/ec2_eni/tasks/test_eni_basic_creation.yaml:

      - _interface_0.private_ip_address | ansible.netcommon.ipaddr

and

      - _interface_0.private_ip_address | ansible.netcommon.ipaddr

My thinking here is that the appropriate validation is done by the subsequent lines:

      - _interface_0.private_ip_address == ip_1

and

      - _interface_0.private_ip_address == ip_5

Would that be acceptable?

tremble · 2021-04-26T07:28:39Z

ansible.netcommon.ipaddr feels like something we should have available to us during testing, many of our modules return IP addresses, and being able to validate this seems like core testing functionality.

Originally we had

ansible.netcommon
community.general
community.aws

While I think it's reasonable to minimise our test dependencies, supported collections (like ansible.netcommon) feel like something we should have available to us.

@goneri thoughts?

ichekaldin · 2021-04-29T17:28:07Z

I realize that everyone is probably busy after release 1.5.0, but still wanted to follow up on this. What's the best way to proceed?

jillr · 2021-04-29T21:32:21Z

@tremble @ichekaldin I have no objection to adding ansible.netcommon back to tests/requirements.yml. It looks like these tests was missed when we recently removed ansible.netcommon from the requirements after other, unrelated usages were removed. Collections in that file will be installed to the test environment when integration tests run.

ichekaldin · 2021-05-12T13:22:35Z

@jillr @tremble What can we do to move this forward? Thanks!

jillr · 2021-05-14T18:56:06Z

recheck

goneri · 2021-05-14T22:11:09Z

recheck

ichekaldin · 2021-06-07T13:00:33Z

@jillr @goneri Is something blocking this PR?

jillr

Hi @ichekaldin apologies, just needed to be reviewed once the CI was passing.

jillr · 2021-06-18T17:34:12Z

recheck

jillr · 2021-06-18T19:31:08Z

gate job isn't running because one of the zuul changes hasn't merged, we'll get that merged asap

AWS jobs depend on ansible.netcommon See: ansible-collections/amazon.aws#337 (comment)

ichekaldin · 2021-07-05T15:00:49Z

@jillr Looks like the zuul changes are merged. Could this be merged as well?

tremble · 2021-07-05T19:02:18Z

@ichekaldin - Apologies for any spam, trying to kick the CI

tremble · 2021-07-05T19:03:47Z

'check' was automatically cancelled because gate's now running - as long as gate passes it should merge.

ichekaldin · 2021-07-07T02:59:08Z

@tremble looks like the gate test fails with this:

TASK [ec2_eni : Assert that ec2_eni_info returns all the values we expect] *****
task path: /home/zuul/.ansible/collections/ansible_collections/amazon/aws/tests/integration/targets/ec2_eni/tasks/test_eni_basic_creation.yaml:24
fatal: [testhost]: FAILED! => {
    "msg": "The conditional check '_interface_0.private_ip_address | ansible.netcommon.ipaddr' failed. The error was: The ipaddr filter requires python's netaddr be installed on the ansible controller"
}

ichekaldin · 2021-07-08T16:43:36Z

@tremble Thank you!

ichekaldin changed the title ~~Fix idempotency when security_groups is specified~~ ec2_eni: Fix idempotency when security_groups is specified Apr 23, 2021

ansibullbot added bug This issue/PR relates to a bug community_review module module needs_triage new_contributor Help guide this first time contributor plugins plugin (any type) small_patch Hopefully easy to review labels Apr 23, 2021

ichekaldin added 2 commits April 24, 2021 23:40

Minor edit (mostly to retrigger tests)

550ad26

ipaddr filter has been moved ansible.netcommon collection

5968ca3

ansibullbot added integration tests/integration tests tests labels Apr 25, 2021

Attempt to explicitly specify ansible.netcommon.ipaddr() instead of j…

73067dc

…ust ipaddr()

ansibullbot removed the small_patch Hopefully easy to review label Apr 25, 2021

Minor edit

359a554

ansibullbot removed the needs_triage label Apr 26, 2021

goneri added the CI label May 14, 2021

goneri mentioned this pull request May 14, 2021

AWS jobs depend on ansible.netcommon ansible/ansible-zuul-jobs#929

Merged

jillr approved these changes Jun 18, 2021

View reviewed changes

jillr added gate and removed gate labels Jun 18, 2021

ansible-zuul Bot pushed a commit to ansible/ansible-zuul-jobs that referenced this pull request Jun 24, 2021

AWS jobs depend on ansible.netcommon (#929)

5bd83ba

AWS jobs depend on ansible.netcommon See: ansible-collections/amazon.aws#337 (comment)

tremble removed the gate label Jul 5, 2021

tremble closed this Jul 5, 2021

tremble reopened this Jul 5, 2021

tremble added the gate label Jul 5, 2021

jillr mentioned this pull request Jul 7, 2021

Install ec2_eni test requirements #398

Merged

tremble removed the gate label Jul 8, 2021

tremble closed this Jul 8, 2021

tremble reopened this Jul 8, 2021

tremble added the gate label Jul 8, 2021

ansible-zuul Bot merged commit 922514a into ansible-collections:main Jul 8, 2021

ichekaldin deleted the ec2_eni/fix_idempotency_security_groups branch July 8, 2021 16:43

Uh oh!

Conversation

ichekaldin commented Apr 23, 2021 • edited by tremble Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

SUMMARY

ISSUE TYPE

COMPONENT NAME

ADDITIONAL INFORMATION

Uh oh!

ansibullbot commented Apr 23, 2021

Uh oh!

ichekaldin commented Apr 23, 2021

Uh oh!

goneri commented Apr 23, 2021

Uh oh!

goneri commented Apr 23, 2021

Uh oh!

ichekaldin commented Apr 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tremble commented Apr 26, 2021

Uh oh!

ichekaldin commented Apr 29, 2021

Uh oh!

jillr commented Apr 29, 2021

Uh oh!

ichekaldin commented May 12, 2021

Uh oh!

jillr commented May 14, 2021

Uh oh!

goneri commented May 14, 2021

Uh oh!

ichekaldin commented Jun 7, 2021

Uh oh!

jillr left a comment

Choose a reason for hiding this comment

Uh oh!

jillr commented Jun 18, 2021

Uh oh!

jillr commented Jun 18, 2021

Uh oh!

ichekaldin commented Jul 5, 2021

Uh oh!

tremble commented Jul 5, 2021

Uh oh!

tremble commented Jul 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ichekaldin commented Jul 7, 2021

Uh oh!

ichekaldin commented Jul 8, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ichekaldin commented Apr 23, 2021 •

edited by tremble

Loading

ichekaldin commented Apr 25, 2021 •

edited

Loading

tremble commented Jul 5, 2021 •

edited

Loading