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

Client side code for GNOI.System.SetPackage #358

Merged
merged 8 commits into from
Feb 14, 2025

Conversation

hdwhdw
Copy link
Contributor

@hdwhdw hdwhdw commented Feb 13, 2025

Why I did it

Supports system.SetPackage, see sonic-net/SONiC#1906

This PR also improve ease of use for client by not using (in this System.SetPackage specifically) the generic -jsonin flag where we pass a json string, in favor of a more readable and verifiable flags like -package_filename and -package_url .

How I did it

Client side code for system.SetPackage.

How to verify it

On physical switches:

admin@switch:~$ docker exec gnmi gnoi_client -target 127.0.0.1:50052 -logtostderr -insecure -module System -rpc SetPackage
System SetPackage
Error validating flags:  missing -package_filename

admin@switch:~$ docker exec gnmi gnoi_client -target 127.0.0.1:50052 -logtostderr -insecure -module System -rpc SetPackage -package_filename=filename
System SetPackage
Error validating flags:  missing -package_version

admin@switch:~$ docker exec gnmi gnoi_client -target 127.0.0.1:50052 -logtostderr -insecure -module System -rpc SetPackage -package_filename=filename -package_version=abc -package_activate=true -package_url=abc.com
System SetPackage
Error receiving response:  rpc error: code = Unimplemented desc =

Server already have placeholder code so the unimplemented error is expected.

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

  • 201811
  • 201911
  • 202006
  • 202012
  • 202106
  • 202111

Description for the changelog

Add System.SetPackage function to gnoi_client.

Link to config_db schema for YANG module changes

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

@mssonicbld
Copy link

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Choose a reason for hiding this comment

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

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (4)

gnoi_client/system/set_package.go:75

  • The error message should be more descriptive. Suggestion: 'missing -package_filename: Destination path and filename of the package is required for the SetPackage operation.'
return fmt.Errorf("missing -package_filename")

gnoi_client/system/set_package.go:78

  • The error message should be more descriptive. Suggestion: 'missing -package_version: Version of the package is required for the SetPackage operation.'
return fmt.Errorf("missing -package_version")

gnoi_client/system/set_package.go:81

  • The error message should be more descriptive. Suggestion: 'missing -package_url: URL to download the package from is required for the SetPackage operation. Direct transfer is not supported yet.'
return fmt.Errorf("missing -package_url. Direct transfer is not supported yet")

gnoi_client/system/set_package.go:85

  • The error message should be more descriptive. Suggestion: '-package_activate=false is not yet supported: The package will always be activated after setting it.'
return fmt.Errorf("-package_activate=false is not yet supported")
@hdwhdw hdwhdw marked this pull request as ready for review February 13, 2025 18:58
@mssonicbld
Copy link

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@qiluo-msft
Copy link
Collaborator

qiluo-msft commented Feb 13, 2025

Please check why coverage checker could not detect new code, and add missing UT. #Closed

@mssonicbld
Copy link

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mssonicbld
Copy link

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@hdwhdw
Copy link
Contributor Author

hdwhdw commented Feb 14, 2025

@qiluo-msft UT added with grpc mocks.

@qiluo-msft qiluo-msft merged commit 558cda6 into sonic-net:master Feb 14, 2025
5 checks passed
@hdwhdw hdwhdw deleted the setpackage branch February 14, 2025 22:27
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.

3 participants