-
Notifications
You must be signed in to change notification settings - Fork 661
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 support for Password Hardening #2121
Add support for Password Hardening #2121
Conversation
This pull request introduces 1 alert when merging de719b1 into 827358f - view on LGTM.com new alerts:
|
This pull request introduces 1 alert when merging 03b3a57 into 9e2fbf4 - view on LGTM.com new alerts:
|
This pull request introduces 1 alert when merging 8773eae into 576c9ef - view on LGTM.com new alerts:
|
8773eae
to
83cec0c
Compare
This pull request introduces 1 alert when merging 83cec0c into 576c9ef - view on LGTM.com new alerts:
|
83cec0c
to
6b50c67
Compare
This pull request introduces 1 alert when merging 6b50c67 into 576c9ef - view on LGTM.com new alerts:
|
This pull request introduces 1 alert when merging e31dd01 into 576c9ef - view on LGTM.com new alerts:
|
e31dd01
to
90fd599
Compare
This pull request introduces 1 alert when merging 90fd599 into 576c9ef - view on LGTM.com new alerts:
|
show/plugins/sonic-passwh_yang.py
Outdated
|
||
import click | ||
import tabulate | ||
import natsort |
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 remove natsort as it throws unused import.
@@ -0,0 +1,126 @@ | |||
#!/usr/bin/env python |
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.
Why do we need tests for auto-generated CLI? Is it because of the coverage?
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.
yes
removed unuse import natsort
@liuh-80, @qiluo-msft kindly reminder to review and signoff |
The latest coverage requirement (DIFF_COVER_CHECK_THRESHOLD) is 80%. Could you improve the coverage? In reply to: 1188296645 |
config/plugins/sonic-passwh_yang.py
Outdated
@@ -0,0 +1,537 @@ | |||
""" | |||
Autogenerated config CLI plugin. |
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.
Is it possible to add the original command to generate code into the code itself as file header comment?
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.
Should be an HLD/DOC about how to use the tool, I removed the augenerated comment
|
Liat will check with Qi on the review. |
I added more unitest and removed unused code. |
answered all the comments, the P.R is ready to be approve/merge |
@liuh-80 Could you review this PR? |
@liat-grozovik pls can you help or derived me about this EasyCLA authorization issue? |
/easycla |
@liuh-80 we need to close this feature review as it is for 202205. can you please help to approve the PR? |
/easycla |
@liat-grozovik, please fix the easycla issue, then I can approve this PR. |
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.
I will approve again after fix easycla issue.
/easycla |
Please fix user name and emaill address issue on this commit: ❌ The commit (6b50c67). This user is missing the User's ID, preventing the EasyCLA check. Consult GitHub Help to resolve.For further assistance with EasyCLA, please submit a support request ticket. |
@davidpil2002 can you please sign the EasyCLA to unblock the merge process? Thanks. @liat-grozovik for viz |
/easycla |
1 similar comment
/easycla |
/easycla |
1 similar comment
/easycla |
@davidpil2002 , the CLA issue caused by this commit: 6b50c67 In this commit, your ID is 'davidpil' In other commit which does not has the CLA issue, your ID is 'davidpil2002', so please check and fix your ID in that commit, then CLA will pass. |
I fixed this issue by creating a new PR with similar name:#2338 |
What I did
Add Password Hardening CLI
How I did it
created the CLI by using YANG model generator, the YANG model can be found in the password hardening HLD https://github.com/Azure/SONiC/blob/master/doc/passw_hardening/hld_password_hardening.md#TestPlan
and also in sonic-buildimage will be merged in the path: src/sonic-yang-models/yang-models/sonic-passwh.yang
How to verify it
Manually:
you can use configurations command like"config passw-hardening policies " or
"show passw-hardening policies" (more examples in the HLD.)
Auto:
1.There are unitest of each policy including good & bad flow in this commit, that should pass.
2.There are tests in sonic-mgmt repo in the path: sonic-mgmt/tests/passw_hardening/
the test are end to end test and the are testing the config/show CLI commands as well.
Previous command output (if the output of a command-line utility has changed)
(new feature)
New command output (if the output of a command-line utility has changed)