revert channel-scoped semver template expansion to edge-scoped #1813
Conversation
Signed-off-by: grokspawn <[email protected]>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1813 +/- ##
==========================================
+ Coverage 55.27% 55.37% +0.10%
==========================================
Files 136 136
Lines 15976 16003 +27
==========================================
+ Hits 8830 8861 +31
+ Misses 5991 5987 -4
Partials 1155 1155 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
alpha/template/semver/semver.go
Outdated
| // - we only need to watch for arch/kind/x change | ||
| // - we don't need to accumulate skips/replaces, since we're not writing edges for subsequent entries | ||
| lastTuple := entries[len(entries)-1] | ||
| penultimateTuple := entries[len(entries)-2] |
There was a problem hiding this comment.
I couldn't quite tell on my own; is len(entries) guaranteed to be >=1? Just noticing the if entryCount > 1 check is now gone. I see the for would be skipped, but I don't see an early return or other safety check.
There was a problem hiding this comment.
REALLY nice catch @dtfranz. The minimum viable channel will have at least one edge, but it's not guaranteed to have more than one.
And that was the challenge with this PR, really. Because while we needed to revert the refactored approach, we also needed to ensure we retained the functionality in the subsequent commits... and I missed this one, although it looks like it was also missing in an earlier commit
I added a new test for the boundary condition.
Thanks!
Signed-off-by: grokspawn <[email protected]>
|
/lgtm |
|
[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
openshift-merge-bot
bot
merged commit 24737fb
into
operator-framework:master
3 participants
Description of the change:
revert app code to before refactoring, but enhance testing to address the changes since then
Motivation for the change:
in #1169 I was attempting to address a semver catalog template expansion error which ignored an X/arch/kind transition on the last edge. In discussion at the time, we had consensus that it only made sense to generate replaces links from minor version bundles to previous minor version head (meaning the previous minor version, maximum patch version, hereafter referred to as "prev-Z-max") in the context of generating major-version channels from the semver template.
The thinking had been that we felt that folks would want to exert controls on generated minor channels and wouldn't want generated prev-Z-max linkage.
That enabled some code refactoring for simplicity, which did not include the prev-Z-max linkage for generated minor version channels.
It turns out in retrospect that we like prev-Z-max for generated minor channels, and folks have asked for it to be re-added. Unfortunately (for me), we have addressed several issues in the same area so we couldn't merely revert to the old commit.
This PR includes the older implementation with tests that validate/enforce the prev-Z-max for generated minor version channels.
Reviewer Checklist
/docs