-
Notifications
You must be signed in to change notification settings - Fork 29
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(upgrade): add unsupported upgrade version file upgrade #229
feat(upgrade): add unsupported upgrade version file upgrade #229
Conversation
1b4ca71
to
da687f9
Compare
k8s/upgrade-job/src/helm/upgrade.rs
Outdated
let mut invalid_upgrade_path: PathBuf; | ||
|
||
match self.upgrade_path_dir { | ||
Some(up_path) => invalid_upgrade_path = up_path, | ||
None => { | ||
return FilePathNotPresent.fail(); | ||
} | ||
} | ||
invalid_upgrade_path.push("unsupported_versions.yaml"); |
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.
let mut invalid_upgrade_path: PathBuf; | |
match self.upgrade_path_dir { | |
Some(up_path) => invalid_upgrade_path = up_path, | |
None => { | |
return FilePathNotPresent.fail(); | |
} | |
} | |
invalid_upgrade_path.push("unsupported_versions.yaml"); | |
let invalid_upgrade_path = match self.upgrade_path_dir { | |
Some(path) => Ok(path.join("unsupported_versions.yaml")), | |
None => Err(Error::FilePathNotPresent), | |
}?; |
Also, should "unsupported_versions.yaml" be a const?
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.
passing the complete file path now.
k8s/upgrade-job/src/opts.rs
Outdated
@@ -41,10 +41,18 @@ pub(crate) struct CliArgs { | |||
)] | |||
core_chart_dir: Option<PathBuf>, | |||
|
|||
/// This is the Helm chart directory filepath for the core Helm chart variant. | |||
#[arg(long, env = "UPGRADE_EXCEPTION_PATH", value_name = "DIR PATH")] | |||
upgrade_exception_dir: Option<PathBuf>, |
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.
why not specify the file path, rather than the dir? And have the default value here so it's clear?
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.
passing the complete file path now.
5594bdd
to
6debe22
Compare
k8s/upgrade-job/src/helm/upgrade.rs
Outdated
@@ -134,18 +153,30 @@ impl HelmUpgradeBuilder { | |||
} | |||
|
|||
// Validating upgrade path. | |||
let invalid_upgrade_path = match self.upgrade_path_file { |
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 we not default it in the cli argument to our file?
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.
done
@@ -0,0 +1,3 @@ | |||
unsupported_versions: | |||
# add the list of unsupported versions as shown below | |||
- 0.0.0 |
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.
Should we add v1 for completeness?
Do we support "wild cards" here btw?
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.
no wild cards support , the version in chart.yal is 0.0.0 so v1 not added here
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.
might be worth adding that, even as a follow-up PR
Signed-off-by: sinhaashish <[email protected]>
6debe22
to
a56391a
Compare
#[arg( | ||
long, | ||
env = "UPGRADE_EXCEPTION_FILE_PATH", | ||
default_value = "/k8s/upgrade/config/unsupported_versions.yaml" |
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: default_value is left for backwards compatibility. Better to use default_value_t.
This value should be in the constants file... and the constant should be used here.
bors merge |
Build succeeded! The publicly hosted instance of bors-ng is deprecated and will go away soon. If you want to self-host your own instance, instructions are here. If you want to switch to GitHub's built-in merge queue, visit their help page. |
230: Cherry pick PR #229 to release/2.2 branch r=sinhaashish a=sinhaashish This PR cherry-picks the following commit(s) to release/2.2: - [a56391a](a56391a) Co-authored-by: sinhaashish <[email protected]>
This PR incorporates the below changes:
--skip-upgrade-path-validation-for-unsupported-version
Test case 1
Install version
2.0.0
and add2.0.0
in unsupported_versions.yaml as shown belowUpgrade fails stating
Test case 2
Using the
skip-upgrade-path-validation-for-unsupported-version
so that upgrade against can be tested.Test case3
Upgrade to develop
Run
/target/debug/kubectl-mayastor upgrade --skip-upgrade-path-validation-for-unsupported-version
Upgrade succeeds