-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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 upgrade test into E2E tests #4058
Add upgrade test into E2E tests #4058
Conversation
520b17b
to
a420207
Compare
b228c1d
to
6a8e716
Compare
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.
Thanks for this, @danfengliu! This is looking great so far :) I've done a lot of upgrade testing recently so I know this is a really complex test to write so thank you for taking this on :)
I have a few comments and suggestions. My main request is that we should be using the upgrade instructions from the website as I don't think calling install
again will replace the CRDs and update the deployment. Also, I think that it should be the responsibility of the runner of these tests to provide the binary to use rather than this code downloading it.
I think the flow of the test should be
- Install with the upgrade-from binary (and don't set the image to use, let the binary use the default image)
- Perform the backup using upgrade-from binary
- Upgrade by explicitly installing the new CRDs and upgrading the image in the deployment/daemonset using the CLI and image that are being tested (the PR/nightly build). In future we should consider adding an "upgrade" command to the CLI to improve this user experience, but that's a problem for another time :)
- Use the new CLI that's being tested to perform the restore.
I hope my comments makes sense! Let me know if anything is unclear 👍
@danfengliu Please resolve the conflict either |
204aa81
to
78df815
Compare
181986a
to
36d415e
Compare
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.
Thanks for making the suggested changes :) I have a few more comments but it's looking good 👍
23a2266
to
2cc69d8
Compare
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.
Thanks
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.
Thanks for this, @danfengliu! I am happy for this to be merged, I just want to check if we can go back to using the upstream version of kibishii rather than a fork.
714693e
to
f4f1276
Compare
Signed-off-by: danfengl <[email protected]>
f4f1276
to
4db866a
Compare
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.
LGTM - thanks!
Signed-off-by: danfengl [email protected]
Thank you for contributing to Velero!
Please add a summary of your change
Add upgrade test in E2E test.
Does your change fix a particular issue?
Fixes #4029
Please indicate you've done the following:
/kind changelog-not-required
.site/content/docs/main
.