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: import dependancy from upgrade module to cosmovisor module #10442

Merged
merged 8 commits into from
Nov 9, 2021

Conversation

yaruwangway
Copy link
Contributor

Description

Closes: #10378

Cosmovisor module copies the data type UpgradeInfo and constant upgradeFilename from upgrade module to cosmovisor module.
This pr will directly import the datatype and const from cosmos-sdk/upgrade module:
UpgradeInfo -> upgradetypes.Plan
upgradeFilename -> upgradekeeper.UpgradeInfoFilename


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)

Sorry, something went wrong.

@yaruwangway yaruwangway added this to the cosmovisor v2.0 milestone Oct 26, 2021
@yaruwangway yaruwangway added the C:Cosmovisor Issues and PR related to Cosmovisor label Oct 26, 2021
@yaruwangway yaruwangway changed the title refactor refactor: import dependancy from upgrade module to cosmovisor module Oct 26, 2021

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@codecov
Copy link

codecov bot commented Oct 26, 2021

Codecov Report

Merging #10442 (0ef11b3) into master (b5ca8e0) will increase coverage by 0.13%.
The diff coverage is 72.41%.

❗ Current head 0ef11b3 differs from pull request most recent head dec316a. Consider uploading reports for the commit dec316a to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10442      +/-   ##
==========================================
+ Coverage   64.20%   64.33%   +0.13%     
==========================================
  Files         581      592      +11     
  Lines       55206    56008     +802     
==========================================
+ Hits        35446    36035     +589     
- Misses      17742    17911     +169     
- Partials     2018     2062      +44     
Impacted Files Coverage Δ
crypto/keys/secp256k1/secp256k1_nocgo.go 83.33% <ø> (ø)
simapp/simd/cmd/genaccounts.go 61.83% <ø> (-0.29%) ⬇️
store/v2/smt/store.go 61.36% <ø> (ø)
types/denom.go 70.00% <ø> (ø)
x/mint/abci.go 0.00% <0.00%> (ø)
x/mint/types/genesis.go 0.00% <0.00%> (ø)
store/v2/flat/store.go 90.84% <11.11%> (ø)
x/auth/middleware/basic.go 65.34% <25.00%> (ø)
cosmovisor/process.go 54.54% <50.00%> (ø)
x/nft/keeper/grpc_query.go 70.89% <70.89%> (ø)
... and 25 more

@robert-zaremba robert-zaremba added the A:automerge Automatically merge PR once all prerequisites pass. label Oct 28, 2021
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

Nice job @yaruwangway 👏

@alexanderbez
Copy link
Contributor

@AmauryM is the liveness test known to not be working?

Binary needs to be OS linux, ARCH amd64

@robert-zaremba
Copy link
Collaborator

For the context. Initially we intentionally didn't want to import SDK in cosmovisor. But with a plan for v2.0 we have a strong dependency to the x/upgrade module and we decided to import it.

@mergify mergify bot merged commit 2394266 into master Nov 9, 2021
@mergify mergify bot deleted the yaru/cosmovisor-dependency branch November 9, 2021 14:15
blewater pushed a commit to e-money/cosmos-sdk that referenced this pull request Dec 8, 2021
…osmos#10442)

<!--
The default pull request template is for types feat, fix, or refactor.
For other templates, add one of the following parameters to the url:
- template=docs.md
- template=other.md
-->

## Description

Closes: cosmos#10378

Cosmovisor module copies the data type `UpgradeInfo` and constant `upgradeFilename` from upgrade module to cosmovisor module.
This pr will directly import the datatype and const from cosmos-sdk/upgrade module:
`UpgradeInfo` ->  `upgradetypes.Plan`
`upgradeFilename` -> `upgradekeeper.UpgradeInfoFilename`
<!-- Add a description of the changes that this PR introduces and the files that
are the most critical to review. -->

---

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

- [x] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] added `!` to the type prefix if API or client breaking change
- [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting))
- [ ] provided a link to the relevant issue or specification
- [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules)
- [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing)
- [ ] added a changelog entry to `CHANGELOG.md`
- [ ] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [ ] 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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) 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)
)

replace github.com/gogo/protobuf => github.com/regen-network/protobuf v1.3.3-alpha.regen.1
Copy link
Member

Choose a reason for hiding this comment

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

this breaks go install, Is there any way we can have cosmovisor not rely on upgrade?

Copy link
Collaborator

@dwedul-figure dwedul-figure Feb 17, 2022

Choose a reason for hiding this comment

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

Cosmovisor and the x/upgrade module are intrinsically linked. Removing that link would require the duplication of lots of logic and structures. And making x/upgrade dependant on Cosmovisor is backwards since the module is part of an SDK, but Cosmovisor is not.

I'm not sure what this replace is actually providing, though.

@tac0turtle tac0turtle mentioned this pull request Feb 17, 2022
19 tasks
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:Cosmovisor Issues and PR related to Cosmovisor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clean up cosmovisor to use x/upgrade as a dependency
7 participants