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 LDAP feature CLI support #3022

Merged
merged 6 commits into from
May 15, 2024

Conversation

davidpil2002
Copy link
Contributor

@davidpil2002 davidpil2002 commented Oct 23, 2023

What I did

Add LDAP CLI

How I did it

created the CLI by using YANG model generator, the YANG model can be found in the LDAP HLD:
sonic-net/SONiC#1487

How to verify it

Manually:
you can use configurations command like"config ldap global " or
"show ldap global" (more examples in the HLD.)
Auto:
1.There are unitest of each policy including good & bad flow in this commit, that should pass.

Previous command output (if the output of a command-line utility has changed)

No prev commands - New feature

New command output (if the output of a command-line utility has changed)

sudo show ldap global 
BIND DN                       BIND PASSWORD      BIND TIMEOUT  LDAP VERSION    BASE DN            LDAP PORT      TIMEOUT
----------------------------  ---------------  --------------  --------------  -----------------  -----------  ---------
cn=ldapadm,dc=test1,dc=test2  password            2              3             dc=test1,dc=test2  389                  2

@davidpil2002 davidpil2002 force-pushed the dev-add-ldap-support branch 2 times, most recently from bdd2580 to 475e92a Compare October 30, 2023 12:09
@liat-grozovik
Copy link
Collaborator

@lguohan who should review and provide feedback?

show_ldap_global="""\
BIND DN BIND PASSWORD BIND TIMEOUT VERSION BASE DN PORT TIMEOUT
---------------------------- --------------- -------------- --------- ----------------- ------ ---------
cn=ldapadm,dc=test1,dc=test2 password 3 3 dc=test1,dc=test2 389 2
Copy link
Contributor

Choose a reason for hiding this comment

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

what are the units for BIND TIMEOUT and LDAP TIMEOUT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

seconds

Copy link

Choose a reason for hiding this comment

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

Is it possible to mention in the header (secs)?

validate_config_or_raise(cfg)
db.set_entry(table, key, None)


Copy link
Contributor

Choose a reason for hiding this comment

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

It is auto generated code, code cleanup must be done.

body.append(row)

click.echo(tabulate.tabulate(body, header))

Copy link
Contributor

Choose a reason for hiding this comment

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

Auto generated code, refactor and code clean up must be done

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The code was autogenerated by using sonic-cli-gen tool
fixed spaces as you mentioned,
pls comment if there is some other issue.
thanks

@davidpil2002 davidpil2002 force-pushed the dev-add-ldap-support branch 3 times, most recently from 2dcee1f to ede8825 Compare December 25, 2023 13:47
@fastiuk
Copy link
Contributor

fastiuk commented Jan 29, 2024

@madhupalu , please approve if everything looks good

@davidpil2002 davidpil2002 force-pushed the dev-add-ldap-support branch 2 times, most recently from cc99892 to f80960d Compare February 29, 2024 07:59
@davidpil2002 davidpil2002 force-pushed the dev-add-ldap-support branch from f80960d to e7b795c Compare March 5, 2024 08:34
@davidpil2002 davidpil2002 force-pushed the dev-add-ldap-support branch from aea3d0e to 5a27799 Compare March 18, 2024 10:21
@qiluo-msft
Copy link
Contributor

@madhupalu Do you have more comments?

Copy link
Collaborator

@liat-grozovik liat-grozovik left a comment

Choose a reason for hiding this comment

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

In order to merge it @davidpil2002 @Yarden-Z please check command reference guide and update it with the new additional CLIs. https://github.com/sonic-net/sonic-utilities/blob/master/doc/Command-Reference.md#lldp

Copy link
Collaborator

@liat-grozovik liat-grozovik left a comment

Choose a reason for hiding this comment

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

need to update command reference guide

@davidpil2002 davidpil2002 force-pushed the dev-add-ldap-support branch from 5a27799 to 3069f8b Compare May 9, 2024 06:06
@davidpil2002
Copy link
Contributor Author

need to update command reference guide

rebase and update Command-Reference.md file

@davidpil2002 davidpil2002 force-pushed the dev-add-ldap-support branch 6 times, most recently from b1efed5 to 9da844f Compare May 10, 2024 11:02
@davidpil2002 davidpil2002 force-pushed the dev-add-ldap-support branch from 9da844f to 5d09fdd Compare May 10, 2024 11:26
@davidpil2002 davidpil2002 force-pushed the dev-add-ldap-support branch from 5d09fdd to 9435879 Compare May 10, 2024 11:30
@liat-grozovik liat-grozovik merged commit 2fda252 into sonic-net:master May 15, 2024
7 checks passed
arfeigin pushed a commit to arfeigin/sonic-utilities that referenced this pull request Jun 16, 2024
- What I did
Add LDAP CLI

- How I did it
created the CLI by using YANG model generator, the YANG model can be found in the LDAP HLD:
sonic-net/SONiC#1487

- How to verify it
Manually:
you can use configurations command like"config ldap global " or
"show ldap global" (more examples in the HLD.)
Auto:
1.There are unitest of each policy including good & bad flow in this commit, that should pass.
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.

6 participants