-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
[release-tooling] Implement a parser from yaml #5562
Conversation
use aptos_types::on_chain_config::OnChainConsensusConfig; | ||
use move_model::{code_writer::CodeWriter, emit, emitln, model::Loc}; | ||
|
||
pub fn generate_consensus_upgrade_proposal( |
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 logic is migrated from #5527. Maybe we could refactor that test later cc @igor-aptos
@@ -0,0 +1,472 @@ | |||
--- |
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 is an example release config that contains all the information we could set using on chain config. Governance proposals could be generated automatically from this yaml file.
89fd2a4
to
20e7d03
Compare
aptos-move/aptos-release-builder/src/components/consensus_config.rs
Outdated
Show resolved
Hide resolved
aptos-move/aptos-release-builder/src/components/consensus_config.rs
Outdated
Show resolved
Hide resolved
|
||
emitln!(writer, "// Consensus config upgrade proposal\n"); | ||
|
||
if is_testnet { |
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.
nit: This is getting a bit redundant. Refactor this so each component doesn't have to do this themselves?
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.
Yea I think I can refactor a bit here.
|
||
pub fn parse(serialized: &str) -> Result<Self> { | ||
serde_yaml::from_str(serialized).map_err(|e| anyhow!("Failed to parse the config: {:?}", e)) | ||
} | ||
} | ||
|
||
impl Default for ReleaseConfig { | ||
fn default() -> Self { |
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.
Can you remind me where this is used again?
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 is used in the upgrade test to make sure that the proposal generation process would always work. Though in the future we should be replacing this with the real release config we want to put for the next release cycle.
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.
"in the future we should be replacing this with the real release config we want to put for the next release cycle." => What do you mean? Isn't this the default only?
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.
Sry I mean that right now, we use ReleaseConfig::default
in the testing pipeline. We should expect to see another test that uses sth like ReleaseConfig::upcoming_release
to make sure the config is indeed valid. Hope that makes sense.
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.
oh this is really nice to have
Updated the PR with the refactor. Tho it doesn't look cleaner than before either... lemme know if you would like to keep it or not cc @movekevin |
20e7d03
to
83b05bb
Compare
83b05bb
to
1c5a2d8
Compare
This is probably beyond this PR, but what is the expected flow? It seems to me release yaml is already materialized release decision - delta between two versions, i.e. what needs to be upgraded, for a specific network (i.e. testnet or not). Correct? I.e. how would we know if any configs have changed, and whether governance proposal for updating onchain config (for example consensus one) is going to be needed? Are we going to have a separate "snapshot" files telling us the state of various networks, and then create a snapshot for new release, and then create there release yamls as a delta between snapshots, for each of the networks? |
Right now the PR only did the generation from yaml path. In the next PR I can add the logic to fetch the config from rest endpoint and compare it against the yaml file. cc @igor-aptos |
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.
makes sense, just wanted to understand where this fits in.
base_path: &Path, | ||
is_testnet: bool, | ||
) -> Result<()> { | ||
pub fn generate_release_proposal_scripts(&self, base_path: &Path) -> Result<()> { | ||
let mut result = vec![]; |
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.
you are not returning the result, or doing anything else with it?
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 function will generate a set of move scripts into the base_path
.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ Forge suite
|
✅ Forge suite
|
* [release-builder] Implement a parser for the framework release config * [release-builder] Add consensus config to release config * [release-tooling] Refactor the common logic for generating proposal.
* [release-builder] Implement a parser for the framework release config * [release-builder] Add consensus config to release config * [release-tooling] Refactor the common logic for generating proposal.
…mainnet (#5936) * [release-tooling] Implement a parser from yaml (#5562) * [release-builder] Implement a parser for the framework release config * [release-builder] Add consensus config to release config * [release-tooling] Refactor the common logic for generating proposal. * [release tooling] multi-step proposal release tooling (#5834) Co-authored-by: chloeqjz <[email protected]> * [aptos-release-tooling] Add an option to compare release binary with on chain configs (#5796) * [aptos-release-tooling] Add an option to compare release binary with on chain configs * fixup! [aptos-release-tooling] Add an option to compare release binary with on chain configs Co-authored-by: 0xchloe <[email protected]> Co-authored-by: chloeqjz <[email protected]>
Description
Implement the parser to parse the release config from a yaml file. Added generation logic for consensus config as well.
Test Plan
cargo run -- write-default --output-path data/example.yaml
and
cargo run -- generate-proposals --release-config data/example.yaml --output-dir <OUTPUT_DIR>
The upgrade smoke test will make sure upgrading consensus config would work as well.
This change is