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

Some fixes in ios_acls module: #998

Merged
merged 13 commits into from
Feb 14, 2024

Conversation

earendilfr
Copy link
Contributor

@earendilfr earendilfr commented Dec 15, 2023

SUMMARY

Some modification done:

  • Currently, when we push an ACL from a type (extended or standard) but an ACL from the other type is present, module dones't delete first the previous ACL.
    So, we could have an error because the ACL are not compatible. The only solution was first delete the previous ACL completely

  • For the version IOS-XE 16.9 and for all the IOS 15.x, the ACE didn't have the sequence number. I fixe was previously done by @KB-perByte but I suspect that something still not working.
    It's better with:

    • added the ^\s* prior both line for ace
    • replace the count variable in config/acls/acls.py by a enumerate function
  • At the same point, I have check the unit test test_ios_acls_parsed_matchesand I think that an "ugly" workaround was put in place to take in count that the ACE was not correctly matched and so, in test, the address object was replaced by a protocol_option object...

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

ios_acls

ADDITIONAL INFORMATION

- Correctly take in count the case where we try to create an ACL with an
  ACL of another type already present.
  I have added the test `test_ios_acls_replaced_changetype` to test it.
- For the ACL without a sequence number, correctly take in count the
  fake ACL number.
  I have also added a test in the unit test test_ios_acls_replaced_idempotent`
  to validate that the test correctly take in count these ACE
- In unit test, the test `test_ios_acls_parsed_matches` was clearly with
  a wrong output format. Fixed it.
Copy link

codecov bot commented Dec 15, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (f4084f2) 86.74% compared to head (3db7186) 86.74%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #998      +/-   ##
==========================================
- Coverage   86.74%   86.74%   -0.01%     
==========================================
  Files         197      197              
  Lines       12067    12081      +14     
==========================================
+ Hits        10468    10480      +12     
- Misses       1599     1601       +2     

see 3 files with indirect coverage changes

@earendilfr earendilfr marked this pull request as ready for review December 15, 2023 14:55
Copy link

Build succeeded.
https://ansible.softwarefactory-project.io/zuul/buildset/1ee858876ee3484cbf17cb22877c884d

✔️ ansible-galaxy-importer SUCCESS in 3m 32s
✔️ build-ansible-collection SUCCESS in 9m 52s
ansible-ee-integration-ios-latest RETRY_LIMIT in 4m 40s (non-voting)
ansible-ee-integration-ios-stable-2.9 FAILURE in 13m 57s (non-voting)
ansible-ee-integration-ios-stable-2.11 RETRY_LIMIT in 3m 13s (non-voting)
ansible-ee-integration-ios-stable-2.12 RETRY_LIMIT in 5m 00s (non-voting)
ansible-ee-integration-ios-libssh-latest RETRY_LIMIT in 3m 33s (non-voting)
ansible-ee-integration-ios-libssh-stable-2.9 FAILURE in 13m 47s (non-voting)
ansible-ee-integration-ios-libssh-stable-2.11 RETRY_LIMIT in 4m 39s (non-voting)
ansible-ee-integration-ios-libssh-stable-2.12 RETRY_LIMIT in 3m 13s (non-voting)
✔️ ansible-tox-linters SUCCESS in 11m 35s

@KB-perByte KB-perByte self-requested a review December 15, 2023 15:56
@KB-perByte KB-perByte added the bug This issue/PR relates to a bug. label Jan 10, 2024
@KB-perByte
Copy link
Collaborator

Hey @earendilfr, I look forward to merging this in upcoming releases, Thank you for your contribution.
Please note - the maximum version of IOS-XE supported by the collection is 17.x, any change native to an older version of IOS would not be considered.
Reagrds

Copy link

Build succeeded.
https://ansible.softwarefactory-project.io/zuul/buildset/f91cfaf06f464e1c850e49a7fb829b53

✔️ ansible-galaxy-importer SUCCESS in 4m 40s
✔️ build-ansible-collection SUCCESS in 11m 02s
ansible-ee-integration-ios-latest RETRY_LIMIT in 3m 57s (non-voting)
ansible-ee-integration-ios-stable-2.9 FAILURE in 14m 18s (non-voting)
ansible-ee-integration-ios-stable-2.11 RETRY_LIMIT in 3m 40s (non-voting)
ansible-ee-integration-ios-stable-2.12 RETRY_LIMIT in 4m 00s (non-voting)
ansible-ee-integration-ios-libssh-latest RETRY_LIMIT in 3m 27s (non-voting)
ansible-ee-integration-ios-libssh-stable-2.9 FAILURE in 16m 16s (non-voting)
ansible-ee-integration-ios-libssh-stable-2.11 RETRY_LIMIT in 4m 02s (non-voting)
ansible-ee-integration-ios-libssh-stable-2.12 RETRY_LIMIT in 3m 58s (non-voting)
✔️ ansible-tox-linters SUCCESS in 11m 51s

KB-perByte and others added 3 commits January 15, 2024 00:40
…#1005)

* revert vlan extra parameters

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* add a changelog

* revert vlans test

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* resource segregate implementation

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* documentation update

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* add basic tests

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* minor doc changes

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* check command options

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* add purge

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* fix unused imports

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* update changelog

* add tests for vlan conf

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* update tests for vlan config purged

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* fix vlan config

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* fix tests

* update documentation

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Update docs 2

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* address review comments

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* update doc

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* update test-req

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Copy link

Build succeeded.
https://ansible.softwarefactory-project.io/zuul/buildset/c9ac74455da34b818f6b2a36d8fa254f

✔️ ansible-galaxy-importer SUCCESS in 4m 56s
✔️ build-ansible-collection SUCCESS in 9m 37s
ansible-ee-integration-ios-latest RETRY_LIMIT in 3m 17s (non-voting)
ansible-ee-integration-ios-stable-2.9 FAILURE in 13m 22s (non-voting)
ansible-ee-integration-ios-stable-2.11 RETRY_LIMIT in 3m 54s (non-voting)
ansible-ee-integration-ios-stable-2.12 RETRY_LIMIT in 3m 17s (non-voting)
ansible-ee-integration-ios-libssh-latest RETRY_LIMIT in 3m 19s (non-voting)
ansible-ee-integration-ios-libssh-stable-2.9 FAILURE in 13m 50s (non-voting)
ansible-ee-integration-ios-libssh-stable-2.11 RETRY_LIMIT in 3m 20s (non-voting)
ansible-ee-integration-ios-libssh-stable-2.12 RETRY_LIMIT in 3m 58s (non-voting)
✔️ ansible-tox-linters SUCCESS in 11m 33s

@KB-perByte
Copy link
Collaborator

@earendilfr can you please look at the conflicts?
I see you have added some tests, we can get it reviewed.

CC @roverflow

earendilfr and others added 5 commits February 1, 2024 19:12
- Correctly take in count the case where we try to create an ACL with an
  ACL of another type already present.
  I have added the test `test_ios_acls_replaced_changetype` to test it.
- For the ACL without a sequence number, correctly take in count the
  fake ACL number.
  I have also added a test in the unit test test_ios_acls_replaced_idempotent`
  to validate that the test correctly take in count these ACE
- In unit test, the test `test_ios_acls_parsed_matches` was clearly with
  a wrong output format. Fixed it.
@earendilfr
Copy link
Contributor Author

@KB-perByte , @roverflow : I have perform the merge and the tests (for ACL module) working fine.
Concerning the unit test added: I remember I have just added a test to simulate the add of an ACL with the same name but a different type than an already existing ACL.

Copy link

Build succeeded.
https://ansible.softwarefactory-project.io/zuul/buildset/9812b170ea854b9aba85e8177d5025f7

✔️ ansible-galaxy-importer SUCCESS in 4m 05s
✔️ build-ansible-collection SUCCESS in 9m 49s
ansible-ee-integration-ios-latest RETRY_LIMIT in 3m 01s (non-voting)
ansible-ee-integration-ios-stable-2.9 FAILURE in 12m 53s (non-voting)
ansible-ee-integration-ios-stable-2.11 RETRY_LIMIT in 3m 07s (non-voting)
ansible-ee-integration-ios-stable-2.12 RETRY_LIMIT in 3m 15s (non-voting)
ansible-ee-integration-ios-libssh-latest RETRY_LIMIT in 3m 14s (non-voting)
ansible-ee-integration-ios-libssh-stable-2.9 FAILURE in 12m 26s (non-voting)
ansible-ee-integration-ios-libssh-stable-2.11 RETRY_LIMIT in 2m 58s (non-voting)
ansible-ee-integration-ios-libssh-stable-2.12 RETRY_LIMIT in 3m 08s (non-voting)
✔️ ansible-tox-linters SUCCESS in 11m 35s

@KB-perByte KB-perByte merged commit 018bf84 into ansible-collections:main Feb 14, 2024
54 of 55 checks passed
@earendilfr earendilfr deleted the ios_acls-few_changes branch February 21, 2024 13:41
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants