-
Notifications
You must be signed in to change notification settings - Fork 64
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
Add LST resource module #447
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This module looks good on first pass. The only notes I have are style based regarding docstring triple quotes being on their own line separated from the function description by a newline.
if all_evpn_es_downstream: | ||
group_dict['all_evpn_es_downstream'] = all_evpn_es_downstream | ||
else: | ||
group_dict['all_evpn_es_downstream'] = False | ||
if all_mclags_downstream: | ||
group_dict['all_mclags_downstream'] = all_mclags_downstream | ||
else: | ||
group_dict['all_mclags_downstream'] = False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dict.get can be used to populate default values.
if all_evpn_es_downstream: | |
group_dict['all_evpn_es_downstream'] = all_evpn_es_downstream | |
else: | |
group_dict['all_evpn_es_downstream'] = False | |
if all_mclags_downstream: | |
group_dict['all_mclags_downstream'] = all_mclags_downstream | |
else: | |
group_dict['all_mclags_downstream'] = False | |
group_dict['all_evpn_es_downstream'] = config.get('all-evpn-es-downstream', False) | |
group_dict['all_mclags_downstream'] = config.get('all-mclags-downstream', False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed.
commands.extend(update_states(have, 'deleted')) | ||
have = {} | ||
|
||
if not have and want: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please handle scenario when 'have' is subset of 'want'. (nothing to delete, but new config is to be added)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed.
There are several modules that don't account for this case so those modules will need to be fixed.
plugins/modules/sonic_lst.py
Outdated
type: list | ||
elements: dict | ||
suboptions: | ||
id: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this option specifies the name of the interface, could it be changed to 'name' ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change set, the accompanying set of tests, and the test results look good. I am posting only a few minor requests for changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for making the requested changes.
All proposed changes, test cases, and corresponding test results look good after the latest commit.
Approved.
SUMMARY
I added a new resource module for link state tracking, LST.
Please see model PR.
ISSUE TYPE
COMPONENT NAME
sonic_lst
OUTPUT
Updated regression results:
regression-2024-12-19-09-04-09.html.pdf
regression-2024-10-14-14-18-15.html.pdf
diff_output.log
merge_check_mode.log
delete_check_mode.log
delete_upstream_group_check_mode.txt
facts_gathering.log
Checklist: