Correct indentation in ACL comparator #564

bewing · 2024-12-30T19:13:31Z

SUMMARY

Change indentation of ACL present check in _state_replaced to only try to add a missing wanted ACL after processing all have ACLs.

Possibly fixes #512

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME

arista.eos.eos_acls

ADDITIONAL INFORMATION

Only check for missing wanted ACLs after processing all had ACLs, not just the first one. Addresses ansible-collections#512

bewing · 2025-02-13T02:08:09Z

@Ruchip16 is there anything needed from my side to move this PR forward?

Ruchip16 · 2025-03-24T11:03:45Z

@Ruchip16 is there anything needed from my side to move this PR forward?

hey thank you for raising the PR, can you kindly add integration tests as well to validate the change on device?

bewing · 2025-03-24T13:46:07Z

I will take a look at the integration test framework this week, thanks!

bewing · 2025-03-25T12:49:57Z

After a few hours screwing around, I was able to generate a Containerlab cEOS image that I can run integration tests on. It appears that this change does affect one of the integration tests:

TASK [eos_acls : ansible.builtin.debug] ****************************************
ok: [clab-ceos-ceos1] => {
    "msg": "Start eos_acls replaced integration tests ansible_connection=ansible.netcommon.network_cli"
}

TASK [eos_acls : ansible.builtin.include_tasks] ********************************
included: /home/bewing/projects/network/naac/ansible_collections/arista/eos/tests/output/.tmp/integration/eos_acls-1h0nrzag-ÅÑŚÌβŁÈ/tests/integration/targets/eos_acls/tests/common/_populate.yaml for clab-ceos-ceos1

TASK [eos_acls : Setup] ********************************************************
changed: [clab-ceos-ceos1]

TASK [eos_acls : ansible.builtin.set_fact] *************************************
ok: [clab-ceos-ceos1]

TASK [eos_acls : replace attributes with given acls.] **************************
changed: [clab-ceos-ceos1]

TASK [eos_acls : arista.eos.eos_facts] *****************************************
ok: [clab-ceos-ceos1]

TASK [eos_acls : ansible.builtin.assert] ***************************************
ok: [clab-ceos-ceos1] => {
    "changed": false,
    "msg": "All assertions passed"
}

TASK [eos_acls : Idempotency check] ********************************************
ok: [clab-ceos-ceos1]

TASK [eos_acls : arista.eos.eos_facts] *****************************************
ok: [clab-ceos-ceos1]

TASK [eos_acls : ansible.builtin.assert] ***************************************
fatal: [clab-ceos-ceos1]: FAILED! => {
    "assertion": "result.changed == true",
    "changed": false,
    "evaluated_to": false,
    "msg": "Assertion failed"
}

TASK [eos_acls : ansible.builtin.include_tasks] ********************************
included: /home/bewing/projects/network/naac/ansible_collections/arista/eos/tests/output/.tmp/integration/eos_acls-1h0nrzag-ÅÑŚÌβŁÈ/tests/integration/targets/eos_acls/tests/common/_remove_config.yaml for clab-ceos-ceos1

TASK [eos_acls : Setup] ********************************************************
changed: [clab-ceos-ceos1]

PLAY RECAP *********************************************************************
clab-ceos-ceos1            : ok=27   changed=6    unreachable=0    failed=1    skipped=0    rescued=0    ignored=0   

NOTICE: To resume at this test target, use the option: --start-at eos_acls
FATAL: Command "ansible-playbook eos_acls-pasd2sq0.yml -i inventory.networking" returned exit status 2.

I'll have to check what the test is supposed to test, and if it was just written to pass incorrectly prior to this change, or if there is a breaking change included in this PR

An idempotency check should ensure that there is NO change and NO commands.

bewing · 2025-03-25T13:23:43Z

After digging into the newly failing integration test, it appears that an idempotency check was incorrectly written to pass when it shouldn't -- if we re-run the same "replaced" operation with the same config as before, we should send no commands, and there should be no changes.

Updated test to reflect.

Ruchip16 · 2025-03-25T13:25:27Z

After a few hours screwing around, I was able to generate a Containerlab cEOS image that I can run integration tests on. It appears that this change does affect one of the integration tests:

TASK [eos_acls : ansible.builtin.debug] ****************************************
ok: [clab-ceos-ceos1] => {
    "msg": "Start eos_acls replaced integration tests ansible_connection=ansible.netcommon.network_cli"
}

TASK [eos_acls : ansible.builtin.include_tasks] ********************************
included: /home/bewing/projects/network/naac/ansible_collections/arista/eos/tests/output/.tmp/integration/eos_acls-1h0nrzag-ÅÑŚÌβŁÈ/tests/integration/targets/eos_acls/tests/common/_populate.yaml for clab-ceos-ceos1

TASK [eos_acls : Setup] ********************************************************
changed: [clab-ceos-ceos1]

TASK [eos_acls : ansible.builtin.set_fact] *************************************
ok: [clab-ceos-ceos1]

TASK [eos_acls : replace attributes with given acls.] **************************
changed: [clab-ceos-ceos1]

TASK [eos_acls : arista.eos.eos_facts] *****************************************
ok: [clab-ceos-ceos1]

TASK [eos_acls : ansible.builtin.assert] ***************************************
ok: [clab-ceos-ceos1] => {
    "changed": false,
    "msg": "All assertions passed"
}

TASK [eos_acls : Idempotency check] ********************************************
ok: [clab-ceos-ceos1]

TASK [eos_acls : arista.eos.eos_facts] *****************************************
ok: [clab-ceos-ceos1]

TASK [eos_acls : ansible.builtin.assert] ***************************************
fatal: [clab-ceos-ceos1]: FAILED! => {
    "assertion": "result.changed == true",
    "changed": false,
    "evaluated_to": false,
    "msg": "Assertion failed"
}

TASK [eos_acls : ansible.builtin.include_tasks] ********************************
included: /home/bewing/projects/network/naac/ansible_collections/arista/eos/tests/output/.tmp/integration/eos_acls-1h0nrzag-ÅÑŚÌβŁÈ/tests/integration/targets/eos_acls/tests/common/_remove_config.yaml for clab-ceos-ceos1

TASK [eos_acls : Setup] ********************************************************
changed: [clab-ceos-ceos1]

PLAY RECAP *********************************************************************
clab-ceos-ceos1            : ok=27   changed=6    unreachable=0    failed=1    skipped=0    rescued=0    ignored=0   

NOTICE: To resume at this test target, use the option: --start-at eos_acls
FATAL: Command "ansible-playbook eos_acls-pasd2sq0.yml -i inventory.networking" returned exit status 2.

I'll have to check what the test is supposed to test, and if it was just written to pass incorrectly prior to this change, or if there is a breaking change included in this PR

kudos that you spawned up an instance to run the integration tests on and yep generally integration tests might break if the changes in PR are not validated hence its a good way to add integration tests and test them on device such that we are sure that we aren't introducing any breaking changes or infact changes that could fail the CI, thanks again and awesome work 💯

bewing · 2025-04-09T18:32:39Z

Any more action required from me for this to be reviewed/merged?

Ruchip16

looks good to me now, thanks for your contribution on this one 💯

* Add unit test for ansible-collections#512 * Correct indentation of ACL comparator Only check for missing wanted ACLs after processing all had ACLs, not just the first one. Addresses ansible-collections#512 * Add changelog entry for replaced acls idempotency * Correct ACL replaced idempotency check An idempotency check should ensure that there is NO change and NO commands. --------- Co-authored-by: Vinay M <[email protected]> Co-authored-by: Ruchi Pakhle <[email protected]> Co-authored-by: Sagar Paul <[email protected]>

bewing had a problem deploying to push December 30, 2024 19:13 — with GitHub Actions Error

pre-commit-ci bot temporarily deployed to push December 30, 2024 19:14 Inactive

bewing force-pushed the iss512 branch from 64eefcf to f882339 Compare December 30, 2024 19:35

bewing temporarily deployed to push December 30, 2024 19:35 — with GitHub Actions Inactive

pre-commit-ci bot temporarily deployed to push December 30, 2024 19:35 Inactive

bewing added 2 commits December 30, 2024 19:37

Add unit test for ansible-collections#512

d8c915d

Correct indentation of ACL comparator

d6286e1

Only check for missing wanted ACLs after processing all had ACLs, not just the first one. Addresses ansible-collections#512

bewing force-pushed the iss512 branch from ba56120 to d6286e1 Compare December 30, 2024 19:40

bewing temporarily deployed to push December 30, 2024 19:40 — with GitHub Actions Inactive

Merge branch 'main' into iss512

abdf0d8

roverflow temporarily deployed to push January 8, 2025 09:47 — with GitHub Actions Inactive

roverflow requested a review from Ruchip16 January 8, 2025 09:48

bewing temporarily deployed to push January 8, 2025 14:08 — with GitHub Actions Inactive

Add changelog entry for replaced acls idempotency

754e76d

bewing force-pushed the iss512 branch from 258e29e to 754e76d Compare January 8, 2025 18:16

bewing temporarily deployed to push January 8, 2025 18:16 — with GitHub Actions Inactive

Merge branch 'main' into iss512

b85ab04

Ruchip16 temporarily deployed to push March 24, 2025 11:02 — with GitHub Actions Inactive

Correct ACL replaced idempotency check

3c2080b

An idempotency check should ensure that there is NO change and NO commands.

bewing temporarily deployed to push March 25, 2025 13:22 — with GitHub Actions Inactive

Merge branch 'main' into iss512

543cd8e

KB-perByte temporarily deployed to push June 5, 2025 13:12 — with GitHub Actions Inactive

Ruchip16 approved these changes Jun 5, 2025

View reviewed changes

Ruchip16 merged commit 5f9357d into ansible-collections:main Jun 5, 2025
50 checks passed

bewing deleted the iss512 branch June 5, 2025 13:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Correct indentation in ACL comparator #564

Correct indentation in ACL comparator #564

Uh oh!

bewing commented Dec 30, 2024

Uh oh!

bewing commented Feb 13, 2025

Uh oh!

Ruchip16 commented Mar 24, 2025

Uh oh!

bewing commented Mar 24, 2025

Uh oh!

bewing commented Mar 25, 2025

Uh oh!

bewing commented Mar 25, 2025

Uh oh!

Ruchip16 commented Mar 25, 2025

Uh oh!

bewing commented Apr 9, 2025

Uh oh!

Ruchip16 left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Correct indentation in ACL comparator #564

Correct indentation in ACL comparator #564

Uh oh!

Conversation

bewing commented Dec 30, 2024

SUMMARY

ISSUE TYPE

COMPONENT NAME

ADDITIONAL INFORMATION

Uh oh!

bewing commented Feb 13, 2025

Uh oh!

Ruchip16 commented Mar 24, 2025

Uh oh!

bewing commented Mar 24, 2025

Uh oh!

bewing commented Mar 25, 2025

Uh oh!

bewing commented Mar 25, 2025

Uh oh!

Ruchip16 commented Mar 25, 2025

Uh oh!

bewing commented Apr 9, 2025

Uh oh!

Ruchip16 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants