feat!: update x/upgrade to handle additional instructions #10032
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -40,6 +40,39 @@ message Plan { | |
| // If this field is not empty, an error will be thrown. | ||
| google.protobuf.Any upgraded_client_state = 5 | ||
| [deprecated = true]; | ||
|
|
||
| // Optional: Upgrade contains additional instructions for the devops or a hypervisor. | ||
| // App specific instructions are handled by the `info` attribute. Here we provide | ||
| // information such as pre-upgrade or post-upgrade commands. | ||
| UpgradeInstructions Upgrade = 6; | ||
|
Comment on lines
+44
to
+47
There was a problem hiding this comment. It might actually be desirable to extract the Also, in order to help minimize on-chain data, should we throw this in a Lastly, I feel like this field would be better named There was a problem hiding this comment.
I was thinking about this part a bit further with extra consideration for the first upgrade using new definitions. If we allow the a url in the The downside is the further overloading of the |
||
| } | ||
|
|
||
| message UpgradeInstructions { | ||
| option (gogoproto.equal) = true; | ||
|
|
||
| // If not empty, a shell command to be run by the upgrade manager or a hypervisor after shutting down | ||
|
There was a problem hiding this comment. I think we should be more clear as to the execution environment of these commands:
There was a problem hiding this comment. Maybe we can drop "shell" and just say "command"? |
||
| // the app and before running a new node. | ||
| string pre_run = 1; | ||
| // If not empty, a shell command to be run by the upgrade manager or a hypervisor after shutting down | ||
| // the app and after running a new app. | ||
| string post_run = 2; | ||
|
There was a problem hiding this comment. Do you know any use case for this? Also how would you deal with the errors on the post upgrade? The application is already running in theory but your post upgrade script fails. What would you do? There was a problem hiding this comment. One possible use-case is reporting. There was a problem hiding this comment. I believe this post-run script can be used for non-critical activities, so we don't need to handle errors. We can use it for removing backups, alerting users etc |
||
| // List of required assets to download. This follows the cosmovisor structure. | ||
| // SHOULD have only one entry per platform. | ||
| repeated Asset assets = 3; | ||
| // Description contains additional information about the upgrade process. Can reference an external resource. | ||
| string description = 4; | ||
|
There was a problem hiding this comment. I think we can remove this If someone provides I guess that if the |
||
| } | ||
|
|
||
| message Asset { | ||
| option (gogoproto.equal) = true; | ||
|
|
||
| // Platform identifier. It's composed from OS and CPU architecture. Example: "linux/amd64" | ||
| string platform = 1; | ||
| // URL to a script or binary to download. Should be related to the UpgradeInstructions pre_run or post_run | ||
| // commands. If multiple files are needed, then they should be packed in a gzip archive. | ||
| string url = 2; | ||
| // Checksum is a sha256 hex encoded checksum of an artifact referenced by the URL. | ||
| string checksum = 3; | ||
|
There was a problem hiding this comment. As we use go-getter to download the files in cosmovisor, it automatically checks for the checksum from the download url as There was a problem hiding this comment. My intention was to make it universal. Proto files should not assume that we are using the go implementation or cosmovisor. We could put the checksum in the URL, but then we would need to explain it in a comment and require it when registering a new proposal. |
||
| } | ||
|
Comment on lines
+66
to
76
There was a problem hiding this comment. With the addition of the I feel like adding a Alternatively, a hierarchy could be used, but there's a couple ways to do that, either name -> platforms or platform -> names. There's good arguments for either way though. But I feel like leaving it flat allows users to handle it the way that makes the most sense for them. Cosmovisor UsageAssets would be downloaded in the order provided but only if there is a If the I'd make a couple special cases for the
We'd probably want to maintain the current functionality where, after doing the downloads, if |
||
|
|
||
| // SoftwareUpgradeProposal is a gov Content type for initiating a software | ||
|
|
||
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.
If you think about a better name for this attribute, please share your proposals. I was also thinking about
Setup,Preparation...We should not confuse with the
Infofield which is already overloaded (both in meaning and usage).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.
Considering it contains both pre and post upgrade info,
UpgradeInstructionssounds good.