Add pull specific solana-release version#3
Conversation
gregcusack
force-pushed
the
setup-tar-deploy
branch
from
8c67f04 to
367a2c0
Compare
joncinque
left a comment
joncinque
left a comment
There was a problem hiding this comment.
Looks good for the most part! We might be able to do something to simplify the CLI args, but otherwise just nits
| let mut file = File::open(path)?; | ||
| let mut contents = String::new(); | ||
| file.read_to_string(&mut contents)?; | ||
| info!("version.yml:\n{}", contents); |
There was a problem hiding this comment.
nit: how about printing the filename?
| info!("version.yml:\n{}", contents); | |
| info!("{}:\n{}", path.file_name(), contents); |
| .arg( | ||
| Arg::with_name("release_channel") | ||
| .long("release-channel") | ||
| .takes_value(true) | ||
| .required_if("deploy_method", "tar") // Require if deploy_method is "tar" | ||
| .help("release version. e.g. v1.17.2. Required if '--deploy-method tar'"), | ||
| ) |
There was a problem hiding this comment.
From my comment on the last review, it might make more sense to change it so that --local-path and --release-channel conflict with each other, then you don't need --deploy-method
And in fact, the DeployMethod enum could look something like:
enum DeployMethod {
Local(String),
ReleaseChannel(String),
Skip,
}
| if tmp_extract_dir.exists() { | ||
| let _ = fs::remove_dir_all(&tmp_extract_dir); | ||
| } |
There was a problem hiding this comment.
When would this be used? if extract_dir is actually a path dir, then tmp-extract should never exist
There was a problem hiding this comment.
ya you're right. will fix
| let file_name = "solana-release"; | ||
| match self.setup_tar_deploy(file_name).await { | ||
| Ok(tar_directory) => { | ||
| info!("Sucessfuly setup tar file"); |
There was a problem hiding this comment.
nit typo
| info!("Sucessfuly setup tar file"); | |
| info!("Successfully setup tar file"); |
| } | ||
|
|
||
| async fn setup_tar_deploy(&self, file_name: &str) -> Result<PathBuf, Box<dyn Error>> { | ||
| let tar_file = format!("{}{}", file_name, ".tar.bz2"); |
There was a problem hiding this comment.
nit: you could simplify this to
| let tar_file = format!("{}{}", file_name, ".tar.bz2"); | |
| let tar_file = format!("{file_name}.tar.bz2"); |
| matches.is_present("skip_build"), | ||
| matches.is_present("debug_build"), |
There was a problem hiding this comment.
Separately, you could also have an enum for BuildType, something like:
enum Build {
Skip,
Debug,
Release,
}
And then you could avoid a situation where someone incorrectly specifies "skip" and "debug"
| match self.deploy_method { | ||
| DeployMethod::Tar => { | ||
| return Err(boxed_error!("Tar deploy method not implemented yet.")); | ||
| let file_name = "solana-release"; |
There was a problem hiding this comment.
nit: Will this always be hardcoded? If so, it might be simpler to just hardcode it in setup_tar_deploy
|
|
||
| async fn download_release_from_channel(&self, file_name: &str) -> Result<(), Box<dyn Error>> { | ||
| info!("Downloading release from channel: {}", self.release_channel); | ||
| let tar_file = format!("{}{}", file_name, ".tar.bz2"); |
There was a problem hiding this comment.
nit: Rather than reconstructing the tar filename, how about passing it in directly as tar_filename instead of file_name?
| }; | ||
|
|
||
| #[macro_export] | ||
| macro_rules! boxed_error { |
There was a problem hiding this comment.
nit: Sorry, I didn't comment on this in the other PR, but rather than using a boxed_error macro and creating an std::io::Error, you can just cast a string as a dyn Error.
There are some examples of that in the token-cli, ie https://github.com/solana-labs/solana-program-library/blob/ce8e4d565edcbd26e75d00d0e34e9d5f9786a646/token/cli/src/command.rs#L1346
joncinque
left a comment
joncinque
left a comment
There was a problem hiding this comment.
Just two more tiny ones, but nothing that should stop this from going in. Looks great, and thanks for applying the feedback!
| .group( | ||
| ArgGroup::new("required_group") | ||
| .args(&["local_path", "release_channel"]) | ||
| .required(true), |
There was a problem hiding this comment.
Since you have the group, I think you can remove the conflicts_with bit actually
There was a problem hiding this comment.
oh good point. will update in next PR
| let build_type: BuildType = matches | ||
| .value_of_t("build_type") | ||
| .unwrap_or_else(|e| e.exit()); |
There was a problem hiding this comment.
micro-nit: you should be able to just unwrap() here since it has a default value
There was a problem hiding this comment.
yep cool call will update in next PR. thank you!
2 participants
Summary of Changes
3rd PR in a series of PRs that will build out the mongon testing framework for deploying validator clusters on Kubernetes
Previous PRs:
Download specific release version from
release.solana.com. pass in a release-channel via--release-channel <release-version: e.g. v1.17.28>. will download into this repo