New modules nxos_tms_global and nxos_tms_destgroup #58018
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@mikewiebe this PR contains more than one new module. Please submit only one new module per pull request. For a detailed explanation, please read the grouped modules documentation |
|
The test The test The test The test The test The test The test The test The test The test The test The test The test The test The test The test |
| protocol = cmd_ref._ref['destination']['existing'][key]['protocol'].lower() | ||
| encoding = cmd_ref._ref['destination']['existing'][key]['encoding'].lower() | ||
| cmd_ref._ref['destination']['existing'][key]['protocol'] = protocol | ||
| cmd_ref._ref['destination']['existing'][key]['encoding'] = encoding |
There was a problem hiding this comment.
Consider this approach instead. It's a bit more readable.
playval = cmd_ref._ref.get('destination').get('playval')
existing = cmd_ref._ref.get('destination').get('existing')
keys = ['protocol', 'encoding']
if playval:
for key in keys:
playval[key] = playval[key].lower()
if existing:
for key in keys:
existing[key] = existing[key].lower()
There was a problem hiding this comment.
This code mostly worked as is but had to modify existing to account for the fact that each existing instance has an instance index.
| cmd_ref._ref['destination']['existing'][key]['protocol'] = protocol | ||
| cmd_ref._ref['destination']['existing'][key]['encoding'] = encoding | ||
|
|
||
| return cmd_ref |
There was a problem hiding this comment.
I don't think you need to return cmd_ref.
|
|
||
| def get_aggregate_cmds(module): | ||
| ''' Get list of commands from aggregate parameter ''' | ||
|
|
There was a problem hiding this comment.
Nit: a little wordy. Consider:
params = module.params
| if 'protocol' in k: | ||
| module.params['destination']['protocol'] = v | ||
| if 'encoding' in k: | ||
| module.params['destination']['encoding'] = v |
There was a problem hiding this comment.
This is more terse:
if 'destination' in k:
+ dest = {}
for k, v in v.items():
+ dest[k] = v
+ params['destination'] = dest
- if 'ip' in k:
- module.params['destination']['ip'] = v
- if 'port' in k:
- module.params['destination']['port'] = v
- if 'protocol' in k:
- module.params['destination']['protocol'] = v
- if 'encoding' in k:
- module.params['destination']['encoding'] = v
There was a problem hiding this comment.
Good refactor suggestion
| cmd_ref.get_playvals() | ||
| cmd_ref = normalize_data(cmd_ref) | ||
| cmds = cmd_ref.get_proposed() | ||
| proposed_cmds.extend(cmds) |
There was a problem hiding this comment.
I'm okay with the code above, just wondering if it would be good to disambiguate the multi-instance cmd_ref objects in this method's for loop from the single-instance cmd_ref in main() ?
e.g. make these mi_cmd_ref, agg_cmd_ref, etc.
Your call.
There was a problem hiding this comment.
Nah.. I plan to leave it as is. It's in a separate method so I think it's clear enough and besides the cmd_ref objects in get_aggregate_cmds are discarded and only the resulting command set is returned.
There was a problem hiding this comment.
👍
yup, just discombobulating the combobulated.
| # Normalize interface name. | ||
| int = module.params.get('destination_profile_source_interface') | ||
| if int: | ||
| module.params['destination_profile_source_interface'] = normalize_interface(int) |
There was a problem hiding this comment.
Or just:
int = module.params.get('destination_profile_source_interface')
module.params['destination_profile_source_interface'] = normalize_interface(int)
It's a nit. Keep it if you want.
| @@ -0,0 +1,3 @@ | |||
| dependencies: | |||
| # Uncomment in the future if needed. | |||
| # - prepare_nxos_tests | |||
There was a problem hiding this comment.
Why skip prepare_nxos_tests ?
There was a problem hiding this comment.
We don't explicitly need it right now and it just adds runtime. That will likely change in the future if/when we add additional nxos platform support but decided to save the time for now.
| @@ -0,0 +1,3 @@ | |||
| dependencies: | |||
| # Uncomment in the future if needed. | |||
| # - prepare_nxos_tests | |||
| # Assumes feature telemetry is disabled | ||
| # TMS global config is not present. | ||
| # Configure only vrf | ||
| module_name = self.module.__name__.rsplit('.', 1)[1] |
There was a problem hiding this comment.
module_name isn't used anywhere else. Remove?
274295f to
c841370
Compare
1f727c7 to
b815f5e
Compare
|
The test The test The test The test The test The test |
|
The test The test The test The test The test The test The test The test The test |
|
The test The test The test The test |
|
@trishnaguha This one is ready for review and merge. @chrisvanheuveln Reviewed it and I finished addressing all of his comments. |
|
shipit |
|
Closing. Redundant with #59126 |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
SUMMARY
CmdRef.ISSUE TYPE
COMPONENT NAME
nxos_tms_global
nxos_tms_destgroup
TESTING INFORMATION
This PR includes both unit and integration tests. Tests have been run against N9k.
A future PR will include support for other NX-OS platforms.