Skip to content

Conversation

@croos12
Copy link
Contributor

@croos12 croos12 commented Nov 18, 2025

Fixes sonic-db-cli argument parsing skipping namespace argument by explicitly setting the options in getopt to select the next space delimitated argument to be namespace and container_name

@mssonicbld
Copy link
Collaborator

/azp run

@azure-pipelines
Copy link

Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command.

@mssonicbld
Copy link
Collaborator

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@croos12 croos12 changed the title Fix sonic-db-cli argument parsing missing namespace [ sonic-db-cli] Fix argument parsing missing namespace Nov 18, 2025
@croos12 croos12 changed the title [ sonic-db-cli] Fix argument parsing missing namespace [sonic-db-cli] Fix argument parsing missing namespace Nov 18, 2025
@croos12
Copy link
Contributor Author

croos12 commented Nov 18, 2025

/azpw run

@mssonicbld
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@croos12
Copy link
Contributor Author

croos12 commented Nov 19, 2025

/azpw run

@mssonicbld
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@croos12
Copy link
Contributor Author

croos12 commented Nov 21, 2025

/azpw run

@mssonicbld
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@croos12 croos12 changed the title [sonic-db-cli] Fix argument parsing missing namespace [sonic-db-cli] Fix argument parsing skipping namespace Nov 21, 2025
@croos12
Copy link
Contributor Author

croos12 commented Nov 21, 2025

@qiluo-msft can you review this PR? It fixes a bug that was introduced for multi-asic after this PR: #1070

@qiluo-msft
Copy link
Contributor

How did you test (old code vs this PR)? Could you add unit test?

Copy link
Contributor

@qiluo-msft qiluo-msft left a comment

Choose a reason for hiding this comment

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

Code lgtm. Suggest add test result in PR description and consider adding testcases.

@croos12
Copy link
Contributor Author

croos12 commented Nov 21, 2025

/azpw run

@mssonicbld
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@croos12
Copy link
Contributor Author

croos12 commented Nov 24, 2025

@qiluo-msft we have unit tests that should cover this, but it looks like the behavior is not consistent. I had to test it by running it directly on the switch with print statements (this switch was a smartswitch so it had dpu databases but not asics) and I could see that the tool was not populating the namespace variable from the parameter :

image

@mssonicbld
Copy link
Collaborator

/azp run

@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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants