Add new cli to add a interface ip as secondary address#3016
Conversation
|
|
||
| if secondary: | ||
| # We update the secondary flag only in case of VLAN Interface. | ||
| if table_name == "VLAN_INTERFACE": |
There was a problem hiding this comment.
For other interface types, do we need to throw an error?
There was a problem hiding this comment.
Do we need a validation to make sure not all IPs are marked as secondary?
There was a problem hiding this comment.
Do we need a validation to make sure no more than one primary IP for a given VLAN interface?
There was a problem hiding this comment.
Do we need a validation to make sure not all IPs are marked as secondary?
Ack, this is a good check.
Do we need a validation to make sure no more than one primary IP for a given VLAN interface?
This we cannot do for two reasons: a) impact to potential existing devices with more than one interface b) interchanging primary with secondary would need more than one primary intermittently.
There was a problem hiding this comment.
This has been addressed. Only vlans with atleast one primary can be added with secondaries.
tests/ip_config_test.py
Outdated
| result = runner.invoke(config.config.commands["interface"].commands["ip"].commands["add"], ["Vlan100", "10.11.20.1/24", "--secondary"], obj=obj) | ||
| assert result.exit_code == 0 | ||
| assert ('Vlan100', '10.11.20.1/24') in db.cfgdb.get_table('VLAN_INTERFACE') | ||
| assert db.cfgdb.get_table('VLAN_INTERFACE')[('Vlan100', '10.11.20.1/24')]['secondary'] == "true" |
There was a problem hiding this comment.
nit: add a test where 'secondary' is false for non-secondary cli call.
There was a problem hiding this comment.
Added the test case for this scenario.
…ot added to the interface
|
@venkatmahalingam can you review the PR? |
|
@shbalaku-microsoft please make sure the new CLI is updated to https://github.com/sonic-net/sonic-utilities/blob/master/doc/Command-Reference.md |
|
@venkatmahalingam , can you signoff? |
|
@shbalaku-microsoft
|
|
@qiluo-msft can we merge this PR? Thanks. |
|
Please merge latest master branch. The semgrep checker is added after this PR raised, so it will always block. |
|
@shbalaku-microsoft , can you please rebase the code? |
|
@shbalaku-microsoft kind reminder to rebase the code, so that we can merge this feature before 202405 fork date on 5/30 |
…utilities into add-cli-command-for-specifying-secondary
#### What I did Added a new CLI command to add secondary subnet value. #### How I did it In the config class, added a snippet of code where the code to validate the secondary field and set the appropriate flag. #### How to verify it Added a test to validate the added flag
#### What I did Added a new CLI command to add secondary subnet value. #### How I did it In the config class, added a snippet of code where the code to validate the secondary field and set the appropriate flag. #### How to verify it Added a test to validate the added flag
7 participants
What I did
Added a new CLI command to add secondary subnet value.
How I did it
In the config class, added a snippet of code where the code to validate the secondary field and set the appropriate flag.
How to verify it
Added a test to validate the added flag
Previous command output (if the output of a command-line utility has changed)
New command output (if the output of a command-line utility has changed)