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.

Sign up for GitHub

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

Jump to bottom

[Local-users-passwords-reset] Add new feature including config, yang and tests #18874

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

[Local-users-passwords-reset] Add new feature including config, yang and tests #18874

wants to merge 3 commits into from

Conversation

azmy98
Copy link

@azmy98 azmy98 commented May 6, 2024 •
edited
Loading

Depends on

sonic-net/sonic-platform-common#465

Why I did it

Added local-users-passwords-reset feature-related services according to HLD: sonic-net/SONiC#1577

Work item tracking

Microsoft ADO (number only):

How I did it

  1. Added reset-local-users-passwords SystemD service
  2. YANG model for new ConfDB table and YANG model tests.
  3. Added config variable responsible for including the new service introduced

How to verify it

Manual test

Which release branch to backport (provide reason below if selected)

  • 201811
  • 201911
  • 202006
  • 202012
  • 202106
  • 202111
  • 202205
  • 202211
  • 202305

Tested branch (Please provide the tested image version)

Description for the changelog

Link to config_db schema for YANG module changes

A picture of a cute animal (not mandatory but encouraged)

@azmy98 azmy98 requested review from qiluo-msft and lguohan as code owners May 6, 2024 04:28
@azmy98 azmy98 force-pushed the dev-reset-local-users-password branch 2 times, most recently from d051a6d to a9ec9e9 Compare May 6, 2024 04:31
@azmy98 azmy98 mentioned this pull request May 6, 2024
Merged
@azmy98 azmy98 force-pushed the dev-reset-local-users-password branch 5 times, most recently from 194a5a0 to 5b81bed Compare May 6, 2024 06:04
@linux-foundation-easycla Linux Foundation: EasyCLA
Copy link

linux-foundation-easycla bot commented May 6, 2024 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

@azmy98 azmy98 force-pushed the dev-reset-local-users-password branch 14 times, most recently from f1f9c87 to a848ace Compare May 7, 2024 10:44
@azmy98 azmy98 requested a review from xumia as a code owner May 7, 2024 10:44
@azmy98 azmy98 force-pushed the dev-reset-local-users-password branch 4 times, most recently from 432ef6e to 96119a9 Compare May 7, 2024 18:08
@azmy98 azmy98 force-pushed the dev-reset-local-users-password branch 2 times, most recently from c1689a0 to 320f7bd Compare May 27, 2024 15:35
@azmy98
Copy link
Author

azmy98 commented May 27, 2024

/azpw ms_conflict

@azmy98 azmy98 force-pushed the dev-reset-local-users-password branch from 320f7bd to b887b6f Compare August 12, 2024 15:21
@azmy98
Copy link
Author

azmy98 commented Sep 23, 2024

/azpw run Azure.sonic-buildimage

@mssonicbld
Copy link
Collaborator

/AzurePipelines run Azure.sonic-buildimage

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

liuh-80
liuh-80 reviewed Sep 25, 2024
View reviewed changes
src/sonic-yang-models/tests/files/sample_config_db.json
@@ -2686,6 +2691,11 @@
}
}
},
"LOCAL_USERS_PASSWORDS_RESET": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this table dupe in config DB?

Copy link
Author

Choose a reason for hiding this comment

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

what do you suggest?

liuh-80
liuh-80 reviewed Sep 25, 2024
View reviewed changes
src/sonic-yang-models/yang-models/sonic-local-users-passwords-reset.yang
description "First Revision";
}

container sonic-local-users-passwords-reset {
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a "FEATURE" table for every feature config, maybe consider using that table?

Copy link
Author

Choose a reason for hiding this comment

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

why don't we need this table as well, this is the template right?

liuh-80
liuh-80 reviewed Sep 25, 2024
View reviewed changes
slave.mk Outdated Show resolved Hide resolved
@azmy98 azmy98 force-pushed the dev-reset-local-users-password branch from b887b6f to f5ea07d Compare November 27, 2024 09:34
FengPan-Frank
FengPan-Frank reviewed Dec 3, 2024
View reviewed changes
files/build_templates/sonic_debian_extension.j2
@@ -626,6 +626,15 @@ sudo cp $IMAGE_CONFIGS/hostname/hostname-config.service $FILESYSTEM_ROOT_USR_LIB
echo "hostname-config.service" | sudo tee -a $GENERATED_SERVICE_FILE
sudo cp $IMAGE_CONFIGS/hostname/hostname-config.sh $FILESYSTEM_ROOT/usr/bin/

{% if enable_local_users_passwords_reset == "y" %}
Copy link
Contributor

Choose a reason for hiding this comment

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

do you need check sonic_asic_platform == "mellanox" as well?

Copy link
Author

Choose a reason for hiding this comment

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

No, why? it is not related to Mellanox or ASIC.. it is a general feature

Copy link
Contributor

Choose a reason for hiding this comment

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

ok I see, but I only saw local_users_passwords_reset.py for mellanox.

Copy link
Author

Choose a reason for hiding this comment

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

The implementation is for Mellanox, each vendor should add their implementation in case they want to use it same as we did in mellanox

@azmy98 azmy98 force-pushed the dev-reset-local-users-password branch from 5204429 to 657df13 Compare December 16, 2024 15:20
@mssonicbld
Copy link
Collaborator

/azp run Azure.sonic-buildimage

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@azmy98 azmy98 force-pushed the dev-reset-local-users-password branch from f8dfa4e to 0d51e81 Compare December 16, 2024 15:23
@azmy98 azmy98 force-pushed the dev-reset-local-users-password branch from 0d51e81 to afc5f19 Compare December 16, 2024 15:24
@mssonicbld
Copy link
Collaborator

/azp run Azure.sonic-buildimage

1 similar comment
@mssonicbld
Copy link
Collaborator

/azp run Azure.sonic-buildimage

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

1 similar comment
@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mssonicbld
Copy link
Collaborator

/azp run Azure.sonic-buildimage

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FengPan-Frank FengPan-Frank FengPan-Frank left review comments

@liuh-80 liuh-80 liuh-80 left review comments

@maipbui maipbui maipbui left review comments

@dgsudharsan dgsudharsan dgsudharsan approved these changes

@qiluo-msft qiluo-msft Awaiting requested review from qiluo-msft qiluo-msft is a code owner

@lguohan lguohan Awaiting requested review from lguohan lguohan is a code owner

@xumia xumia Awaiting requested review from xumia xumia is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@azmy98 @fastiuk @mssonicbld @dgsudharsan @maipbui @liuh-80 @FengPan-Frank @azmyali98