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

feat: update x/upgrade keeper.DumpUpgradeInfoToDisk to support Plan.Info #10532

Merged
merged 3 commits into from
Nov 15, 2021

Conversation

robert-zaremba
Copy link
Collaborator

Description

Closes: #10531
Closes: #10428

Straw-man attempt to bring the plan.Info data in to upgrade-info.json file in a backward compatible way.


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

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

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

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

// `info` should be provided and contain Plan.Info data in order to support
// auto upgrade functionality by cosmovisor and other tools using upgarde-info.json
// (GetUpgradeInfoPath()) file.
func (k Keeper) DumpUpgradeInfoToDisk(height int64, name string, info ...string) error {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

trick to make the API backward compatible.

Copy link
Member

Choose a reason for hiding this comment

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

Why don't we just add another overload DumpFullUpgradeInfoToDisk? The varargs feels too hacky.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The master branch already has it as DumpUpgradeInfoToDisk(height int64, p types.Plan) error. This PR is just to get the functionality into v0.44.x.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The question was if we should do it by adding new function (and instantly marking it "deprecated") or overloading the existing DumpUpgradeInfoToDisk.
In general overloading smells bad, so Aaron comment makes sense.

Copy link
Collaborator

@dwedul-figure dwedul-figure left a comment

Choose a reason for hiding this comment

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

LGTM

// `info` should be provided and contain Plan.Info data in order to support
// auto download functionality by cosmovisor and other tools using upgarde-info.json
// (GetUpgradeInfoPath()) file.
func (k Keeper) DumpUpgradeInfoWithInfoToDisk(height int64, name string, info string) error {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I hate this kind of names.... but couldn't find better one

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like it either lol. What about *WithPlan? Also, why is a new function being introduced and marked as deprecated?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The whole thing is a Plan (name, height, info...).
In original solution I overloaded the DumpUpgradeInfoToDisk using additional vararg (that made it backward compatible). I agree with Aaron, that overloading doesn't smell good. see: #10532 (comment)

return k.DumpUpgradeInfoWithInfoToDisk(height, name, "")
}

// Deprecated: DumpUpgradeInfoWithInfoToDisk writes upgrade information to UpgradeInfoFileName.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

adding a new function which is deprecated

Choose a reason for hiding this comment

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

Ok

@robert-zaremba robert-zaremba added the A:automerge Automatically merge PR once all prerequisites pass. label Nov 15, 2021
@robert-zaremba robert-zaremba merged commit 6a46c95 into release/v0.44.x Nov 15, 2021
@robert-zaremba robert-zaremba deleted the robert/upgrade-info-strawman branch November 15, 2021 22:45
nnkken pushed a commit to nnkken/cosmos-sdk that referenced this pull request Jan 16, 2022
…nfo (cosmos#10532)

* feat: update x/upgrade keeper.DumpUpgradeInfoToDisk to support Plan.Info

* add changelog

* create DumpUpgradeInfoWithInfoToDisk instead of overloading DumpUpgradeInfoToDisk
JeancarloBarrios pushed a commit to agoric-labs/cosmos-sdk that referenced this pull request Sep 28, 2024
…nfo (cosmos#10532)

* feat: update x/upgrade keeper.DumpUpgradeInfoToDisk to support Plan.Info

* add changelog

* create DumpUpgradeInfoWithInfoToDisk instead of overloading DumpUpgradeInfoToDisk
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:automerge Automatically merge PR once all prerequisites pass. C:x/upgrade
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Include Info in store.UpgradeInfo in 0.44.x upgrade-info.json not dumped correctly
5 participants