-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
"show runningconfiguration command" HLD #838
Conversation
|
||
# Requirements Overview | ||
SONiC provide CLI command to configure needed feature for normal users, but users may hard to idenfity which command has been configured if they aren't familiar with CFG_DB's schema related to the feature. | ||
Use existing **sonic-cfggen** command can help to render back command from reading CFG_DB, this is legacy-like running commands. |
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 this HLD using auto CLI generation design present in sonic-net/sonic-utilities#1644 to show running-config? the reason for asking is, we don't have to write separate jinja templates for running-config if we use the auto CLI generation logic itself for showing the running-config.
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.
No, this enhancement only aims on original SONiC click CLI format rendered from CONFIG_DB.
Before SONiC's CLI migrate to new format, we can have a easy way to let user understand what they configured.
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.
Would this design/implementation be removed, then, once 1644 is merged?
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.
If this enhancement can be merged, I think that it would be helpful for original SONiC click CLI even 1644 is merged.
It can be added/removed easily according to the design/implementation.
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 disagree with the premise of this design. The maintenance burden being imposed on the developer community is, in my mind, unjustified given the stated use case. If this PR is merged, I see two possible outcomes - either we ignore this thing when we build new features (making it less and less relevant over time), or everything slows down for each feature we add while we do additional work to transform structured data into a legacy format.
Unstructured legacy configuration text is a bug, not a feature, in my opinion. I don't think we should be putting effort into keeping it alive.
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 understand the desire to have this information coming from a CLI, I disagree with the implementation. This really should be done using the auto CLI generation design and not legacy CLI. Legacy CLI should only have bug fixes not new features added.
I agree with @venkatmahalingam and @tomammon on this. By not using the auto CLI design, you are placing unnecessary burden and maintenance on future development.
@superchild can you please add the code PRs into this PR? Please remember to add the test PR as well. Thanks. |
@zhangyanzhao according to the reviewer's comment and the SONiC's design in the future, I'll close this PR first, it should follow the new CLI architecture after it's merged. |
Enhancement command to render configurations from DB to SONiC CLI's format HLD