-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
feat: create config fix tool #14342
feat: create config fix tool #14342
Conversation
So I think we can ignore the dependency review for this one, as gin does not affect confix at all. |
tools/confix/migrations.go
Outdated
type MigrationMap map[string]transform.Plan | ||
|
||
var Migrations = MigrationMap{ | ||
"v0.45": nil, |
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.
So, here I wonder what is best, as the tool works standalone as well.
I think there are a few options:
- store the config files directly in the tool
- pro: can work standalone
- con: need to add the file in the tool at each release, as well for minor release with configuration changes
- use the SDK version of the app and marshal to config, and have a diff
- pro: we don't mind about the different versions, as we just diff and add what is missing
- con: as we use the SDK version probably work with only one version, especially standalone
- write migrations per versions
- pro: can work standalone, similar to genesis migrate
- con: need to detect which version you are migrating from (or assuming you can only bump one by one version - like genesis migrate), need to be updated at every configuration change
We can as well combine these options.
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.
Step one is already implemented. Working on supporting 2 (as per #14342 (comment)).
3 duplicates 1 and is more manual.
Adding some docs in the release process as well for 1.
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.
left some comments. some things are unclear to me
tools/confix/cmd/migrate.go
Outdated
|
||
func MigrateCommand() *cobra.Command { | ||
cmd := &cobra.Command{ | ||
Use: "migrate [target-version] [app-toml-path] (options)", |
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.
im not sure this needs version? if this is part of the rootcmd it would check if the version config of the binary matches the one on disk.
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 wanted to keep the same UX that for genesis migration. So you basically say to which version you want to migrate. This way it works standalone and as well in the rootcmd.
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.
this would be when the user is using the tool standalone, otherwise it would look at the binary right?
I can do that in a follow-up PR, as it increases the size of this PR: #14342 (comment). This leaves the diff a bit more readable. Then we can tag only when the follow-up PR is merged. Wdyt @tac0turtle? Fixing the last tests, but the sonarcloud key should still be added in the repo. |
[Cosmos SDK] Kudos, SonarCloud Quality Gate passed! |
2d6a2f3
to
a6bac20
Compare
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.
utACK
[Cosmos SDK - SimApp] Kudos, SonarCloud Quality Gate passed! |
[Cosmos SDK - Confix] Kudos, SonarCloud Quality Gate passed! |
[Cosmos SDK - Rosetta] Kudos, SonarCloud Quality Gate passed! |
Adding auto-merge, the follow-up will add the diff command and #14342 (comment). |
Description
Closes: #13661, #11051
Still decided to go with the standalone way, so it is re-usable.
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!
to the type prefix if API or client breaking changeCHANGELOG.md
Reviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!
in the type prefix if API or client breaking change