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

Refactor: remove viper in server context #22388

Closed
kocubinski opened this issue Oct 28, 2024 · 4 comments
Closed

Refactor: remove viper in server context #22388

kocubinski opened this issue Oct 28, 2024 · 4 comments
Labels
C:server/v2 Issues related to server/v2

Comments

@kocubinski
Copy link
Member

kocubinski commented Oct 28, 2024

After the merge of #22267 it will be possible to remove the viper.Viper instance from server command context and replace it with server.ConfigMap. This small bit of refactoring abstracts all config loading away from Viper, since that is handled early in application life cycle with ParseCommand. It will allow for:

  1. swapping out config parsing for something else (e.g. viper)
  2. swapping out our CLI lib for another one (e.g. cobra)
  3. more easily provide custom config to tests calling CLI handlers since a viper instance is not needed, which could decrease our dependency on system tests for nearly everything.
@github-project-automation github-project-automation bot moved this to 📋 Backlog in Cosmos-SDK Legacy Oct 28, 2024
@github-actions github-actions bot added the needs-triage Issue that needs to be triaged label Oct 28, 2024
@tac0turtle tac0turtle added C:server/v2 Issues related to server/v2 and removed needs-triage Issue that needs to be triaged labels Oct 29, 2024
@julienrbrt
Copy link
Member

julienrbrt commented Nov 4, 2024

If we do that, we should remove the ViperContextKey from core then, as it will be a v1 thing only.
Maybe we should store the whole config is context instead then.

Additionally, ReadConfig returns a viper, and viper really helps for binding the flags, would you want to do that manually?

@kocubinski
Copy link
Member Author

Agree that ViperContextKey should be renamed to ConfigContextKey and return a server.ConfigMap if we go this route. Also ClientCtx should not have a viper.Viper either. I'm not sure how breaking this, maybe we can PoC it a bit and see if it's worth it?

@julienrbrt
Copy link
Member

Yes, I agree with the above. To not be breaking it should be a DynamicConfig stored in the context however and not a ConfigMap.

@tac0turtle tac0turtle moved this from 📋 Backlog to ☃️ Icebox in Cosmos-SDK Legacy Nov 27, 2024
@julienrbrt
Copy link
Member

Given that we have viper in core v1, and the release is approaching soon and viper is nicely integrated in the servers, closing this as won't do,.

@julienrbrt julienrbrt closed this as not planned Won't fix, can't repro, duplicate, stale Dec 18, 2024
@github-project-automation github-project-automation bot moved this from ☃️ Icebox to 🥳 Done in Cosmos-SDK Legacy Dec 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:server/v2 Issues related to server/v2
Projects
No open projects
Status: 🥳 Done
Development

No branches or pull requests

4 participants