🌱 (fix): Experimental E2E Upgrade does breaks up we do a release when breaking changes on experimental features are merged #2353
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces experimental E2E upgrade testing that tests upgrades from the main branch to PR changes. The implementation adds a new script hack/test/install-from-main.sh that installs OLMv1 from the main branch source code, and updates the Makefile to use this script in the experimental upgrade test flow.
Key Changes
- Adds
install-from-main.shscript to clone, build, and install from the main branch - Creates new
run-main-installMakefile target - Updates
test-upgrade-experimental-e2eto use main branch installation instead of latest release
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| hack/test/install-from-main.sh | New script that clones main branch, builds images, and installs OLMv1 from main for upgrade testing |
| Makefile | Adds run-main-install target and new task list for experimental upgrade E2E tests |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
camilamacedo86
force-pushed
the
fix-ci-workwound-up-next-release
branch
from
197c984 to
7c004f4
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2353 +/- ##
==========================================
- Coverage 74.23% 74.20% -0.03%
==========================================
Files 91 91
Lines 7239 7239
==========================================
- Hits 5374 5372 -2
- Misses 1433 1434 +1
- Partials 432 433 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
camilamacedo86
force-pushed
the
fix-ci-workwound-up-next-release
branch
from
7c004f4 to
b7a693b
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
camilamacedo86
force-pushed
the
fix-ci-workwound-up-next-release
branch
from
b7a693b to
d04d559
Compare
camilamacedo86
changed the title
camilamacedo86
requested review from
anik120,
perdasilva,
rashmigottipati and
tmshort
| echo "Deploying manifests from main branch..." | ||
| # Extract CERT_MGR_VERSION from main branch Makefile | ||
| # Expected Makefile line format: export CERT_MGR_VERSION := <value> | ||
| CERT_MGR_VERSION_FROM_MAIN=$(grep "^export CERT_MGR_VERSION" Makefile | awk -F'[ :=]+' '{print $NF}') |
There was a problem hiding this comment.
if we're anyway in the checked out main branch, why not use some of the make targets like make kind-deploy?
There was a problem hiding this comment.
We need to run the install script (same generated when we do a release)
That’s different from running make kind-deploy.
There was a problem hiding this comment.
Exactly, this is duplicating a lot of existing Makefile behavior.
perdasilva
left a comment
perdasilva
left a comment
There was a problem hiding this comment.
My only concern is that we're re-determining the cert manager version. Maybe we could move some (or all?) of the scripts logic into the Makefile in the future. For now, the most important thing is to unblock CI. So, if there's anything to improve, we can do it as a follow up.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: perdasilva 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 |
openshift-ci
bot
added
the
approved
The CertManager is defined in the Makefile |
| run-latest-release: | ||
| curl -L -s https://github.com/operator-framework/operator-controller/releases/latest/download/$(notdir $(RELEASE_INSTALL)) | bash -s | ||
|
|
||
| .PHONY: run-main-install |
There was a problem hiding this comment.
This duplicates the run-internal target, but with a script.
|
I'm actually going to disagree with this approach. It’s not just main->experimental, there’s two axis here: standard or experimental, and prior-release or main (not-yet-released). With those we can see that we have 4 possibilities… my initial look at the Makefile indicates that for test-upgrade-experimental-e2e, it’s always using the experimental manifest, so it’s doing an experimental upgrade. We seem to be missing a number of interesting possibilities here (note “main” could represent the PR as well).
What we don’t have is the situation where we cross standard and experimental: standard[main] -> experimental[mainPR] (REQUIRED?) This PR just modifies the existing See #2360 as an alternative |
|
Closing in favor of #2360 |
3 participants
This PR updates the experimental upgrade path to avoid failures caused by expected breaking changes in experimental features by moving experimental upgrade tests to main → PR to prevent experimental breakages from failing and forcing us to do a release.
Current issue
The current flow is: latest release → PR, with all experimental features enabled.
If an experimental feature breaks, the upgrade job fails, blocking merges until the next release.
Proposed change
Switch the experimental upgrade flow to: main → PR.
This keeps the stable upgrade path clean while still validating experimental changes against
main.So, we can safe merge a breaking change for experimental and then the follow PRs will pass and we still raising those scenarios to be aware of.