-
Notifications
You must be signed in to change notification settings - Fork 87
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
🌱 Add release 1.1 upgrade test #904
🌱 Add release 1.1 upgrade test #904
Conversation
/test-centos-e2e-integration-main |
/test-e2e-upgrade-main |
/test-centos-e2e-integration-main |
/test-centos-e2e-integration-main |
362e176
to
1dbbb6f
Compare
/test-centos-e2e-integration-main |
/test-e2e-upgrade-main |
1dbbb6f
to
bc0a03e
Compare
/test-centos-e2e-integration-main |
1 similar comment
/test-centos-e2e-integration-main |
/test-e2e-upgrade-main |
1 similar comment
/test-e2e-upgrade-main |
bc0a03e
to
9dda469
Compare
/test-e2e-upgrade-main |
2 similar comments
/test-e2e-upgrade-main |
/test-e2e-upgrade-main |
6e889e7
to
3bb79c4
Compare
/test-e2e-upgrade-main |
/keep-test-e2e-upgrade-main |
3bb79c4
to
108732e
Compare
/keep-test-e2e-upgrade-main |
1 similar comment
/keep-test-e2e-upgrade-main |
108732e
to
59682f5
Compare
/test-e2e-upgrade-main |
85fc344
to
ebe8a02
Compare
/test-e2e-upgrade-ma |
/keep-test-e2e-upgrade-main-from-release-0-5 |
1 similar comment
/keep-test-e2e-upgrade-main-from-release-0-5 |
f92a6e9
to
1f3afbd
Compare
/test-e2e-upgrade-main-from-release-0-5 |
/test-e2e-upgrade-main-from-release-1-1 |
1 similar comment
/test-e2e-upgrade-main-from-release-1-1 |
1f3afbd
to
19b34a8
Compare
/test-e2e-upgrade-main-from-release-0-5 |
cmd := exec.Command("bash", "-c", "kubectl apply -f bmhosts_crs.yaml -n metal3") | ||
cmd.Dir = workDir | ||
output, err := cmd.CombinedOutput() | ||
Logf("Applying bmh to meta3 namespace : \n %v", string(output)) |
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
Logf("Applying bmh to meta3 namespace : \n %v", string(output)) | |
Logf("Applying bmh to metal3 namespace : \n %v", string(output)) |
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
19b34a8
to
6869d77
Compare
/test-e2e-upgrade-main-from-release-0-5 |
|
||
- **/test-e2e-upgrade-main-from-release-0-5** runs e2e upgrade tests from CAPM3 API version v1alpha5/branch release-0.5 to CAPM3 API version v1beta1/branch main on Ubuntu | ||
|
||
- **/test-e2e-upgrade-main-from-release-1-1** runs e2e upgrade tests from CAPM3 API version v1beta1/branch release-1.1 to CAPM3 API version v1beta1/branch main on Ubuntu | ||
|
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 change these phrases and add ubuntu keyword too?
**/test-ubuntu-e2e-upgrade-main-from-release-0-5** runs e2e upgrade tests from CAPM3 API version v1alpha5/branch release-0.5 to CAPM3 API version v1beta1/branch main on Ubuntu | |
- **/test-ubuntu-e2e-upgrade-main-from-release-1-1** runs e2e upgrade tests from CAPM3 API version v1beta1/branch release-1.1 to CAPM3 API version v1beta1/branch main on Ubuntu |
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.
it is already there on line 161
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.
I think Adil meant adding ubuntu in phrase
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.
yes in the phrase.
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.
well this needs some changes in the JJB this just documentation of what we really have there if someone changed it he can update this docs
/lgtm |
/test-centos-e2e-integration-main |
6869d77
to
bd8cd17
Compare
/test-e2e-upgrade-main-from-release-0-5 |
/test-centos-e2e-integration-main |
@mquhuy: adding LGTM is restricted to approvers and reviewers in OWNERS files. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/lgtm |
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.
Few comments inline, I would also like to see a matrix similar to what we have in https://github.com/metal3-io/cluster-api-provider-metal3/blob/main/docs/e2e-test.md to depict which upgrade test is testing what version wise.
# Get latest capi patch for the compatible version with capm3 $UPGRADE_FROM_RELEASE | ||
# for example the compatible version with capm3 0.5.x is capi 0.4.y | ||
# If we are not upgrading from v0.5 then the capi compatible has same minor version tag as capm3 e.g v1.1.x | ||
if [[ "${UPGRADE_FROM_RELEASE:-v0.5.}" == "v0.5." ]]; then |
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 do we need to default it in the if statement when we already defaulted it few lines above?
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 variable is different from the once above
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 the same var actually. Lets check it in a followup PR then, it seems incorrect to me.
# If we are not upgrading from v0.5 then the capi compatible has same minor version tag e.g v1.1.x | ||
CAPI_UPGRADE_FROM_RELEASE=${UPGRADE_FROM_RELEASE} | ||
fi | ||
export CAPI_FROM_RELEASE="${CAPI_FROM_RELEASE:-$(get_latest_release "${CAPIRELEASEPATH}" "${CAPI_UPGRADE_FROM_RELEASE}")}" |
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.
the variable names are really confusing for me, whats the difference between CAPI_FROM_RELEASE
and CAPI_UPGRADE_FROM_RELEASE
, can we change this something bit more clearer and less confusing ?
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.
We propagate only minor version from jjb and we name it = CAPI_FROM_RELEASE
then when get the patch version that we are upgrading to I used var name CAPI_UPGRADE_FROM_RELEASE
I think the best way is to name the first CAPI_FROM_MINOR_RELEASE
and the second CAPI_FROM_PATCH_RELEASE
but that will need changes in jjb and project infra
files: | ||
- sourcePath: "../data/shared/v1beta1/metadata.yaml" | ||
- sourcePath: "../data/shared/${CAPI_TO_RELEASE:0:4}/metadata.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.
is this variable naming convention correct ${CAPI_TO_RELEASE:0:4} ? or is the collon doing some bash trick? where is the v
?
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 just extracting sub-string
/test-centos-e2e-integration-main |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kashifest The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This PR add the changes required to be able to run upgrade tests from different releases to current release like :