Bug(eos_acls): improve condition to not always convert port numbers to IANA assigned names #432
Conversation
| for op, val in ace["source"]["port_protocol"].items(): | ||
| if val.isdigit(): | ||
| val = socket.getservbyport(int(val)) | ||
| command = command + " " + op + " " + val.replace("_", "-") |
There was a problem hiding this comment.
Instead of removing the translation completely, could we keep a list of EOS implemented names as a constant? If the translated name is in the list we keep it, and if not revert to the original value.
There was a problem hiding this comment.
that's a good idea, I will have to check if that list has ever changed first, otherwise that would add a dependency on EOS versions, which I'm not sure if this collection deals with yet
There was a problem hiding this comment.
so it looks like we had various changes along the years so it wouldn't be a trivial change to make, however if users want to use names instead of numbers they could just use names in the yaml input right?
There was a problem hiding this comment.
upon further investigation I've found out that we have a base list of port numbers that has been there since day 1 and added all those in a list as a constant and validate if the value is a digit and part of our base list then it can do the translation otherwise it can't, tested on 4.22.4 and 4.30.1
Codecov ReportAll modified lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #432 +/- ##
=======================================
Coverage 82.71% 82.72%
=======================================
Files 153 154 +1
Lines 11950 11955 +5
=======================================
+ Hits 9885 9890 +5
Misses 2065 2065 ☔ View full report in Codecov by Sentry. |
…ive hints about it in another file + add a more meaningful docstring
noredistribution
changed the title
|
can someone from the maintainers add |
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
@noredistribution Thank you for the contribution, I have added a comment, Could you please also add a changelog for this?
|
|
||
| __metaclass__ = type | ||
|
|
||
| PORT_NUMBERS = [ |
There was a problem hiding this comment.
A better place for this would be utils.py https://github.com/ansible-collections/arista.eos/blob/main/plugins/module_utils/network/eos/utils/utils.py
|
@noredistribution Looks like this merge in stuck due to the missing changelog? |
SUMMARY
Fixes #431
ISSUE TYPE
COMPONENT NAME
eos_aclsADDITIONAL INFORMATION
Example playbook:
Before the change:
After the change: