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

[config] CLI command configure interface ip doesn't check correctness of inputs #1381

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

d-dashkov
Copy link
Contributor

- What I did
Before writing the settings to the database, check the correctness of the entries.
If you try to bind the ip of a non-existent entry, show an error.
fixes sonic-net/sonic-buildimage#6358

- How I did it
Added functions that check if an entry from an argument exists in the database.

- How to verify it
Try to configure ip to non-existent entry(Ethernet, Vlan, PortChannel, Loopback):
$ config interface ip add Ethernet0.5 10.0.0.1/24
$ config interface ip add Vlan4095 10.0.0.1/24

- New command output (if the output of a command-line utility has changed)
Error: 'interface_name' is not valid. Valid names [Ethernet/PortChannel/Vlan/Loopback]

Signed-off-by: d-dashkov <Dmytro_Dashkov@Jabil.com>
Signed-off-by: d-dashkov <Dmytro_Dashkov@Jabil.com>
@dprital
Copy link
Collaborator

dprital commented Jun 14, 2021

@d-dashkov - Are you going to handle this PR ? I see that it got a conflict.

@dprital
Copy link
Collaborator

dprital commented Jun 17, 2021

@d-dashkov - Are you going to handle this PR ? I see that it got a conflict.

@d-dashkov ?

@d-dashkov
Copy link
Contributor Author

@d-dashkov - Are you going to handle this PR ? I see that it got a conflict.

@d-dashkov ?

Sorry, I will resolve conflicts within a couple of days

@liat-grozovik
Copy link
Collaborator

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@liat-grozovik
Copy link
Collaborator

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@liat-grozovik
Copy link
Collaborator

@dgsudharsan could you please review this PR?

Copy link
Contributor

@prsunny prsunny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes lgtm, please add some unit-tests

Copy link
Collaborator

@dgsudharsan dgsudharsan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add UT. Please also sync to the latest

Signed-off-by: Mykola Horb <mykola_horb@jabil.com>
dgsudharsan
dgsudharsan previously approved these changes Oct 9, 2021
prsunny
prsunny previously approved these changes Oct 12, 2021
@prsunny
Copy link
Contributor

prsunny commented Oct 12, 2021

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@dgsudharsan
Copy link
Collaborator

@d-dashkov Can you please check the pipeline failures?

Signed-off-by: d-dashkov <Dmytro_Dashkov@Jabil.com>
@d-dashkov d-dashkov dismissed stale reviews from prsunny and dgsudharsan via 473b6b1 December 29, 2021 21:26
@d-dashkov
Copy link
Contributor Author

@dgsudharsan, I have fixed sonic-utilities tests and merge conflicts, but there are errors in pipeline which are not related to my changes, and it is about warm-reboot

@d-dashkov d-dashkov requested a review from prsunny January 8, 2022 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

There should be a validation of inputs for "configure interface ip" command
6 participants