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

Add check to not allow deleting PO if its member of vlan. #2141

Merged
merged 9 commits into from
Jun 2, 2022
Merged

Add check to not allow deleting PO if its member of vlan. #2141

merged 9 commits into from
Jun 2, 2022

Conversation

anilkpan
Copy link
Contributor

What I did

Added check to not allow deleting PortChannel if its member of vlan.

How I did it

Added a check in the command script.

How to verify it

  1. Create a portchannel.
  2. Add the portchannel to a vlan.
  3. Try to delete the portchannel.
  4. It should throw an error message.

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)

Error: PortChannel1 has vlan Vlan100 configured, remove vlan membership to proceed

Added check to not allow deleting PO if its member of vlan.
@anilkpan
Copy link
Contributor Author

Fixes issue #10388

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.

Can you please add UT for coverage?

@anilkpan
Copy link
Contributor Author

I am working on adding UT coverage

@dprital
Copy link
Collaborator

dprital commented May 9, 2022

@anilkpan - Any update for this PR ?

@anilkpan
Copy link
Contributor Author

anilkpan commented May 9, 2022

@dprital, I plan to add the tests by the end of the week.

prsunny
prsunny previously approved these changes May 13, 2022
dgsudharsan
dgsudharsan previously approved these changes May 13, 2022
@anilkpan anilkpan dismissed stale reviews from dgsudharsan and prsunny via 0c7f614 May 13, 2022 17:59
@dgsudharsan
Copy link
Collaborator

@anilkpan Can you please review the pipeline failures?

@anilkpan
Copy link
Contributor Author

@dgsudharsan, I am looking at the failures.

@anilkpan
Copy link
Contributor Author

PortChannel creation was successful, but it says portchannel doesn't exist when trying to add to vlan. Not sure how to fix this.

    # add a portchannel
    result = runner.invoke(config.config.commands["portchannel"].commands["add"], ["PortChannel1005"], obj=obj)
    print(result.exit_code)
    print(result.output)
    assert result.exit_code == 0

    # add portchannel to a vlan
    result = runner.invoke(config.config.commands["vlan"].commands["member"].commands["add"], ["2000", "PortChannel1005"], obj=obj)
    print(result.exit_code)
    print(result.output)
  assert result.exit_code == 0

E assert 2 == 0
E + where 2 = <Result SystemExit(2)>.exit_code

tests/portchannel_test.py:197: AssertionError
----------------------------- Captured stdout call -----------------------------
0

2
Usage: add [OPTIONS] port
Try "add --help" for help.

Error: PortChannel1005 does not exist

@dgsudharsan
Copy link
Collaborator

  assert result.exit_code == 0

Were you able to debug further? I believe you need to have portchannel as a member here https://github.com/Azure/sonic-utilities/blob/6a327adf6b55faed8c4f6ddfb8bde6f80efbef2d/tests/mock_tables/config_db.json. The config commands here may not modify the DB.
Please add vlan and portchannel in the file. Try to perform only remove and check if it helps.

@anilkpan
Copy link
Contributor Author

anilkpan commented Jun 1, 2022

@dgsudharsan, Thanks. I will try this.

@yxieca
Copy link
Contributor

yxieca commented Jun 15, 2022

@dgsudharsan this PR cannot be cherry-picked cleanly to 202205. please open PR if needed in 202205.

@dgsudharsan
Copy link
Collaborator

@anilkpan Can you please raise a backport PR to 202205 branch?

@anilkpan
Copy link
Contributor Author

@dgsudharsan, will create the PR this week.

@yxieca
Copy link
Contributor

yxieca commented Jun 24, 2022

@dgsudharsan , @anilkpan do you have ETA for the 202205 PR?

@anilkpan
Copy link
Contributor Author

@yxieca, I plan to create the PR by Monday.

@anilkpan
Copy link
Contributor Author

Created PR #2237

@dgsudharsan
Copy link
Collaborator

@anilkpan Thanks. However I see this PR in master has a binary file .DS_Store. Was this added by mistake? Can you please remove it?

@anilkpan
Copy link
Contributor Author

@dgsudharsan, yes it was added by mistake, will remove it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants