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

"show runningconfiguration command" HLD #838

Closed
wants to merge 1 commit into from

Conversation

superchild
Copy link
Contributor

Enhancement command to render configurations from DB to SONiC CLI's format HLD

@ghost
Copy link

ghost commented Aug 13, 2021

CLA assistant check
All CLA requirements met.


# 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.
Copy link
Collaborator

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.

Copy link
Contributor Author

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.

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?

Copy link
Contributor Author

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.

Copy link

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.

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.

@zhangyanzhao
Copy link
Collaborator

@superchild can you please add the code PRs into this PR? Please remember to add the test PR as well. Thanks.

@superchild
Copy link
Contributor Author

@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.

@superchild superchild closed this Oct 27, 2021
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.

5 participants