From 2096c4f5d0d2cb3ed31767d406afe10786565f01 Mon Sep 17 00:00:00 2001 From: Jordan Keister Date: Tue, 5 Mar 2024 14:23:36 -0600 Subject: [PATCH] Semver lastedge error (#1169) * when the last edge in a bundle set is +Y the existing code does not clear the skips list and crosses Y thresholds counter to design intent missing proper last-max-z detection for replaces working at long last * re-add cumulative skips over y changes in an x stream so folks can always skip directly to channel head Signed-off-by: Jordan Keister --------- Signed-off-by: Jordan Keister --- alpha/template/semver/semver.go | 134 +++++++++--------- alpha/template/semver/semver_test.go | 204 ++++++++++++++++++++++----- alpha/template/semver/types.go | 14 -- 3 files changed, 233 insertions(+), 119 deletions(-) diff --git a/alpha/template/semver/semver.go b/alpha/template/semver/semver.go index d42afedf3..a580fbc01 100644 --- a/alpha/template/semver/semver.go +++ b/alpha/template/semver/semver.go @@ -6,14 +6,13 @@ import ( "io" "sort" - "github.com/blang/semver/v4" - "k8s.io/apimachinery/pkg/util/errors" - "k8s.io/apimachinery/pkg/util/sets" - "sigs.k8s.io/yaml" - "github.com/operator-framework/operator-registry/alpha/action" "github.com/operator-framework/operator-registry/alpha/declcfg" "github.com/operator-framework/operator-registry/alpha/property" + + "github.com/blang/semver/v4" + "k8s.io/apimachinery/pkg/util/errors" + "sigs.k8s.io/yaml" ) func (t Template) Render(ctx context.Context) (*declcfg.DeclarativeConfig, error) { @@ -224,7 +223,6 @@ func (sv *semverTemplate) generateChannels(semverChannels *bundleVersions) []dec hwc := highwaterChannel{archetype: archetypesByPriority[0], version: semver.Version{Major: 0, Minor: 0}} unlinkedChannels := make(map[string]*declcfg.Channel) - unassociatedEdges := []entryTuple{} for _, archetype := range archetypesByPriority { bundles := (*semverChannels)[archetype] @@ -272,7 +270,6 @@ func (sv *semverTemplate) generateChannels(semverChannels *bundleVersions) []dec } } ch.Entries = append(ch.Entries, declcfg.ChannelEntry{Name: bundleName}) - unassociatedEdges = append(unassociatedEdges, entryTuple{arch: archetype, kind: cKey, parent: cName, name: bundleName, version: bundles[bundleName], index: len(ch.Entries) - 1}) } } } @@ -280,80 +277,77 @@ func (sv *semverTemplate) generateChannels(semverChannels *bundleVersions) []dec // save off the name of the high-water-mark channel for the default for this package sv.defaultChannel = hwc.name - outChannels = append(outChannels, sv.linkChannels(unlinkedChannels, unassociatedEdges)...) + outChannels = append(outChannels, sv.linkChannels(unlinkedChannels, semverChannels)...) return outChannels } -func (sv *semverTemplate) linkChannels(unlinkedChannels map[string]*declcfg.Channel, entries []entryTuple) []declcfg.Channel { +func (sv *semverTemplate) linkChannels(unlinkedChannels map[string]*declcfg.Channel, harvestedVersions *bundleVersions) []declcfg.Channel { channels := []declcfg.Channel{} - // sort to force partitioning by archetype --> kind --> semver - sort.Slice(entries, func(i, j int) bool { - if channelPriorities[entries[i].arch] != channelPriorities[entries[j].arch] { - return channelPriorities[entries[i].arch] < channelPriorities[entries[j].arch] - } - if streamTypePriorities[entries[i].kind] != streamTypePriorities[entries[j].kind] { - return streamTypePriorities[entries[i].kind] < streamTypePriorities[entries[j].kind] - } - return entries[i].version.LT(entries[j].version) - }) - - prevZMax := "" - var curSkips sets.String = sets.NewString() - - for index := 1; index < len(entries); index++ { - prevTuple := entries[index-1] - curTuple := entries[index] - prevX := getMajorVersion(prevTuple.version) - prevY := getMinorVersion(prevTuple.version) - curX := getMajorVersion(curTuple.version) - curY := getMinorVersion(curTuple.version) - - archChange := curTuple.arch != prevTuple.arch - kindChange := curTuple.kind != prevTuple.kind - xChange := !prevX.EQ(curX) - yChange := !prevY.EQ(curY) - - if archChange || kindChange || xChange || yChange { - // if we passed any kind of change besides Z, then we need to set skips/replaces for previous max-Z - prevChannel := unlinkedChannels[prevTuple.parent] - finalEntry := &prevChannel.Entries[prevTuple.index] - finalEntry.Replaces = prevZMax - // don't include replaces in skips list, but they are accumulated in discrete cycles (and maybe useful for later channels) so remove here - if curSkips.Has(finalEntry.Replaces) { - finalEntry.Skips = curSkips.Difference(sets.NewString(finalEntry.Replaces)).List() - } else { - finalEntry.Skips = curSkips.List() - } - } - - if archChange || kindChange || xChange { - // we don't maintain skips/replaces over these transitions - curSkips = sets.NewString() - prevZMax = "" - } else { - if yChange { - prevZMax = prevTuple.name + // bundle --> version lookup + bundleVersions := make(map[string]semver.Version) + for _, vs := range *harvestedVersions { + for b, v := range vs { + if _, ok := bundleVersions[b]; !ok { + bundleVersions[b] = v } - curSkips.Insert(prevTuple.name) } } - // last entry accumulation - lastTuple := entries[len(entries)-1] - prevChannel := unlinkedChannels[lastTuple.parent] - finalEntry := &prevChannel.Entries[lastTuple.index] - finalEntry.Replaces = prevZMax - // don't include replaces in skips list, but they are accumulated in discrete cycles (and maybe useful for later channels) so remove here - if curSkips.Has(finalEntry.Replaces) { - finalEntry.Skips = curSkips.Difference(sets.NewString(finalEntry.Replaces)).List() - } else { - finalEntry.Skips = curSkips.List() - } + for _, channel := range unlinkedChannels { + entries := &channel.Entries + sort.Slice(*entries, func(i, j int) bool { + return bundleVersions[(*entries)[i].Name].LT(bundleVersions[(*entries)[j].Name]) + }) - for _, ch := range unlinkedChannels { - channels = append(channels, *ch) + // "inchworm" through the sorted entries, iterating curEdge but extending yProbe to the next Y-transition + // then catch up curEdge to yProbe as 'skips', and repeat until we reach the end of the entries + // finally, because the inchworm will always fail to pick up the last Y-transition, we test for it and link it up as a 'replaces' + curEdge, yProbe := 0, 0 + zmaxQueue := "" + entryCount := len(*entries) + + for curEdge < entryCount { + for yProbe < entryCount { + curVersion := bundleVersions[(*entries)[curEdge].Name] + yProbeVersion := bundleVersions[(*entries)[yProbe].Name] + if getMinorVersion(yProbeVersion).EQ(getMinorVersion(curVersion)) { + yProbe += 1 + } else { + break + } + } + // if yProbe crossed a threshold, the previous entry is the last of the previous Y-stream + preChangeIndex := yProbe - 1 + + if curEdge != yProbe { + if zmaxQueue != "" { + // add skips edge to allow skipping over y iterations within an x stream + (*entries)[preChangeIndex].Skips = append((*entries)[preChangeIndex].Skips, zmaxQueue) + (*entries)[preChangeIndex].Replaces = zmaxQueue + } + zmaxQueue = (*entries)[preChangeIndex].Name + } + for curEdge < preChangeIndex { + // add skips edges to y-1 from z < y + (*entries)[preChangeIndex].Skips = append((*entries)[preChangeIndex].Skips, (*entries)[curEdge].Name) + curEdge += 1 + } + curEdge += 1 + yProbe = curEdge + 1 + } + // since probe will always fail to pick up a y-change in the last item, test for it + if entryCount > 1 { + penultimateEntry := &(*entries)[len(*entries)-2] + ultimateEntry := &(*entries)[len(*entries)-1] + penultimateVersion := bundleVersions[penultimateEntry.Name] + ultimateVersion := bundleVersions[ultimateEntry.Name] + if ultimateVersion.Minor != penultimateVersion.Minor { + ultimateEntry.Replaces = penultimateEntry.Name + } + } + channels = append(channels, *channel) } return channels diff --git a/alpha/template/semver/semver_test.go b/alpha/template/semver/semver_test.go index 8fe470b6e..148290679 100644 --- a/alpha/template/semver/semver_test.go +++ b/alpha/template/semver/semver_test.go @@ -13,25 +13,26 @@ import ( ) func TestLinkChannels(t *testing.T) { - // type entryTuple struct { - // arch channelArchetype - // kind streamType - // name string - // parent string - // index int - // version semver.Version - // } - - majorChannelEntries := []entryTuple{ - {arch: stableChannelArchetype, kind: majorStreamType, name: "a-v0.1.0", parent: "stable-v0", index: 0, version: semver.MustParse("0.1.0")}, - {arch: stableChannelArchetype, kind: majorStreamType, name: "a-v0.1.1", parent: "stable-v0", index: 1, version: semver.MustParse("0.1.1")}, - {arch: stableChannelArchetype, kind: majorStreamType, name: "a-v1.1.0", parent: "stable-v1", index: 0, version: semver.MustParse("1.1.0")}, - {arch: stableChannelArchetype, kind: majorStreamType, name: "a-v1.2.1", parent: "stable-v1", index: 1, version: semver.MustParse("1.2.1")}, - {arch: stableChannelArchetype, kind: majorStreamType, name: "a-v1.3.1", parent: "stable-v1", index: 2, version: semver.MustParse("1.3.1")}, - {arch: stableChannelArchetype, kind: majorStreamType, name: "a-v2.1.0", parent: "stable-v2", index: 0, version: semver.MustParse("2.1.0")}, - {arch: stableChannelArchetype, kind: majorStreamType, name: "a-v2.1.1", parent: "stable-v2", index: 1, version: semver.MustParse("2.1.1")}, - {arch: stableChannelArchetype, kind: majorStreamType, name: "a-v2.3.1", parent: "stable-v2", index: 2, version: semver.MustParse("2.3.1")}, - {arch: stableChannelArchetype, kind: majorStreamType, name: "a-v2.3.2", parent: "stable-v2", index: 3, version: semver.MustParse("2.3.2")}, + // type bundleVersions map[string]map[string]semver.Version // e.g. d["stable"]["example-operator.v1.0.0"] = 1.0.0 + channelOperatorVersions := bundleVersions{ + "stable": { + "a-v0.1.0": semver.MustParse("0.1.0"), + "a-v0.1.1": semver.MustParse("0.1.1"), + "a-v1.1.0": semver.MustParse("1.1.0"), + "a-v1.2.1": semver.MustParse("1.2.1"), + "a-v1.3.1": semver.MustParse("1.3.1"), + "a-v2.1.0": semver.MustParse("2.1.0"), + "a-v1.3.1-beta": semver.MustParse(("1.3.1-beta")), + "a-v2.1.1": semver.MustParse("2.1.1"), + "a-v2.3.1": semver.MustParse("2.3.1"), + "a-v2.3.2": semver.MustParse("2.3.2"), + "a-v3.1.0": semver.MustParse("3.1.0"), + "a-v3.1.1": semver.MustParse("3.1.1"), + "a-v1.3.1-alpha": semver.MustParse("1.3.1-alpha"), + "a-v1.4.1": semver.MustParse("1.4.1"), + "a-v1.4.1-beta1": semver.MustParse("1.4.1-beta1"), + "a-v1.4.1-beta2": semver.MustParse("1.4.1-beta2"), + }, } majorGeneratedUnlinkedChannels := map[string]*declcfg.Channel{ @@ -67,6 +68,67 @@ func TestLinkChannels(t *testing.T) { }, } + majorGeneratedUnlinkedChannelsLastXChange := map[string]*declcfg.Channel{ + "stable-v0": { + Schema: "olm.channel", + Name: "stable-v0", + Package: "a", + Entries: []declcfg.ChannelEntry{ + {Name: "a-v0.1.0"}, + {Name: "a-v0.1.1"}, + }, + }, + "stable-v1": { + Schema: "olm.channel", + Name: "stable-v1", + Package: "a", + Entries: []declcfg.ChannelEntry{ + {Name: "a-v1.1.0"}, + {Name: "a-v1.2.1"}, + {Name: "a-v1.3.1"}, + }, + }, + "stable-v2": { + Schema: "olm.channel", + Name: "stable-v2", + Package: "a", + Entries: []declcfg.ChannelEntry{ + {Name: "a-v2.1.0"}, + }, + }, + } + + majorGeneratedUnlinkedChannelsLastArchChange := map[string]*declcfg.Channel{ + "candidate-v2": { + Schema: "olm.channel", + Name: "candidate-v2", + Package: "a", + Entries: []declcfg.ChannelEntry{ + {Name: "a-v2.3.2"}, + }, + }, + "stable-v1": { + Schema: "olm.channel", + Name: "stable-v1", + Package: "a", + Entries: []declcfg.ChannelEntry{ + {Name: "a-v1.1.0"}, + {Name: "a-v1.2.1"}, + {Name: "a-v1.3.1"}, + }, + }, + "stable-v2": { + Schema: "olm.channel", + Name: "stable-v2", + Package: "a", + Entries: []declcfg.ChannelEntry{ + {Name: "a-v2.1.0"}, + {Name: "a-v2.1.1"}, + {Name: "a-v2.3.1"}, + }, + }, + } + tests := []struct { name string unlinkedChannels map[string]*declcfg.Channel @@ -94,9 +156,9 @@ func TestLinkChannels(t *testing.T) { Name: "stable-v1", Package: "a", Entries: []declcfg.ChannelEntry{ - {Name: "a-v1.1.0", Replaces: "", Skips: []string{}}, - {Name: "a-v1.2.1", Replaces: "a-v1.1.0", Skips: []string{}}, - {Name: "a-v1.3.1", Replaces: "a-v1.2.1", Skips: []string{"a-v1.1.0"}}, + {Name: "a-v1.1.0", Replaces: ""}, + {Name: "a-v1.2.1", Replaces: "a-v1.1.0", Skips: []string{"a-v1.1.0"}}, + {Name: "a-v1.3.1", Replaces: "a-v1.2.1", Skips: []string{"a-v1.2.1"}}, }, }, { @@ -107,7 +169,78 @@ func TestLinkChannels(t *testing.T) { {Name: "a-v2.1.0", Replaces: ""}, {Name: "a-v2.1.1", Replaces: "", Skips: []string{"a-v2.1.0"}}, {Name: "a-v2.3.1", Replaces: ""}, - {Name: "a-v2.3.2", Replaces: "a-v2.1.1", Skips: []string{"a-v2.1.0", "a-v2.3.1"}}, + {Name: "a-v2.3.2", Replaces: "a-v2.1.1", Skips: []string{"a-v2.1.1", "a-v2.3.1"}}, + }, + }, + }, + }, + { + name: "No edges between successive major channels where last edge is X change", + unlinkedChannels: majorGeneratedUnlinkedChannelsLastXChange, + generateMinorChannels: false, + generateMajorChannels: true, + out: []declcfg.Channel{ + { + Schema: "olm.channel", + Name: "stable-v0", + Package: "a", + Entries: []declcfg.ChannelEntry{ + {Name: "a-v0.1.0", Replaces: ""}, + {Name: "a-v0.1.1", Replaces: "", Skips: []string{"a-v0.1.0"}}, + }, + }, + { + Schema: "olm.channel", + Name: "stable-v1", + Package: "a", + Entries: []declcfg.ChannelEntry{ + {Name: "a-v1.1.0", Replaces: ""}, + {Name: "a-v1.2.1", Replaces: "a-v1.1.0", Skips: []string{"a-v1.1.0"}}, + {Name: "a-v1.3.1", Replaces: "a-v1.2.1", Skips: []string{"a-v1.2.1"}}, + }, + }, + { + Schema: "olm.channel", + Name: "stable-v2", + Package: "a", + Entries: []declcfg.ChannelEntry{ + {Name: "a-v2.1.0", Replaces: ""}, + }, + }, + }, + }, + { + name: "No edges between successive major channels where last edge is archetype change", + unlinkedChannels: majorGeneratedUnlinkedChannelsLastArchChange, + generateMinorChannels: false, + generateMajorChannels: true, + out: []declcfg.Channel{ + { + Schema: "olm.channel", + Name: "candidate-v2", + Package: "a", + Entries: []declcfg.ChannelEntry{ + {Name: "a-v2.3.2", Replaces: ""}, + }, + }, + { + Schema: "olm.channel", + Name: "stable-v1", + Package: "a", + Entries: []declcfg.ChannelEntry{ + {Name: "a-v1.1.0", Replaces: ""}, + {Name: "a-v1.2.1", Replaces: "a-v1.1.0", Skips: []string{"a-v1.1.0"}}, + {Name: "a-v1.3.1", Replaces: "a-v1.2.1", Skips: []string{"a-v1.2.1"}}, + }, + }, + { + Schema: "olm.channel", + Name: "stable-v2", + Package: "a", + Entries: []declcfg.ChannelEntry{ + {Name: "a-v2.1.0", Replaces: ""}, + {Name: "a-v2.1.1", Replaces: "", Skips: []string{"a-v2.1.0"}}, + {Name: "a-v2.3.1", Replaces: "a-v2.1.1", Skips: []string{"a-v2.1.1"}}, }, }, }, @@ -117,7 +250,7 @@ func TestLinkChannels(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { sv := &semverTemplate{pkg: "a", GenerateMajorChannels: tt.generateMajorChannels, GenerateMinorChannels: tt.generateMinorChannels} - require.ElementsMatch(t, tt.out, sv.linkChannels(tt.unlinkedChannels, majorChannelEntries)) + require.ElementsMatch(t, tt.out, sv.linkChannels(tt.unlinkedChannels, &channelOperatorVersions)) }) } } @@ -160,14 +293,14 @@ func TestGenerateChannels(t *testing.T) { Name: "stable-v1", Package: "a", Entries: []declcfg.ChannelEntry{ - {Name: "a-v1.1.0", Replaces: "", Skips: []string{}}, - {Name: "a-v1.2.1", Replaces: "a-v1.1.0", Skips: []string{}}, + {Name: "a-v1.1.0", Replaces: ""}, + {Name: "a-v1.2.1", Replaces: "a-v1.1.0", Skips: []string{"a-v1.1.0"}}, {Name: "a-v1.3.1-alpha", Replaces: ""}, {Name: "a-v1.3.1-beta", Replaces: ""}, - {Name: "a-v1.3.1", Replaces: "a-v1.2.1", Skips: []string{"a-v1.1.0", "a-v1.3.1-alpha", "a-v1.3.1-beta"}}, + {Name: "a-v1.3.1", Replaces: "a-v1.2.1", Skips: []string{"a-v1.2.1", "a-v1.3.1-alpha", "a-v1.3.1-beta"}}, {Name: "a-v1.4.1-beta1", Replaces: ""}, {Name: "a-v1.4.1-beta2", Replaces: ""}, - {Name: "a-v1.4.1", Replaces: "a-v1.3.1", Skips: []string{"a-v1.1.0", "a-v1.2.1", "a-v1.3.1-alpha", "a-v1.3.1-beta", "a-v1.4.1-beta1", "a-v1.4.1-beta2"}}, + {Name: "a-v1.4.1", Replaces: "a-v1.3.1", Skips: []string{"a-v1.3.1", "a-v1.4.1-beta1", "a-v1.4.1-beta2"}}, }, }, { @@ -178,7 +311,7 @@ func TestGenerateChannels(t *testing.T) { {Name: "a-v2.1.0", Replaces: ""}, {Name: "a-v2.1.1", Replaces: "", Skips: []string{"a-v2.1.0"}}, {Name: "a-v2.3.1", Replaces: ""}, - {Name: "a-v2.3.2", Replaces: "a-v2.1.1", Skips: []string{"a-v2.1.0", "a-v2.3.1"}}, + {Name: "a-v2.3.2", Replaces: "a-v2.1.1", Skips: []string{"a-v2.1.1", "a-v2.3.1"}}, }, }, { @@ -207,7 +340,7 @@ func TestGenerateChannels(t *testing.T) { Name: "stable-v1.1", Package: "a", Entries: []declcfg.ChannelEntry{ - {Name: "a-v1.1.0", Replaces: "", Skips: []string{}}, + {Name: "a-v1.1.0", Replaces: ""}, }, }, { @@ -215,7 +348,7 @@ func TestGenerateChannels(t *testing.T) { Name: "stable-v1.2", Package: "a", Entries: []declcfg.ChannelEntry{ - {Name: "a-v1.2.1", Replaces: "a-v1.1.0", Skips: []string{}}, + {Name: "a-v1.2.1", Replaces: ""}, }, }, { @@ -225,7 +358,7 @@ func TestGenerateChannels(t *testing.T) { Entries: []declcfg.ChannelEntry{ {Name: "a-v1.3.1-alpha", Replaces: ""}, {Name: "a-v1.3.1-beta", Replaces: ""}, - {Name: "a-v1.3.1", Replaces: "a-v1.2.1", Skips: []string{"a-v1.1.0", "a-v1.3.1-alpha", "a-v1.3.1-beta"}}, + {Name: "a-v1.3.1", Replaces: "", Skips: []string{"a-v1.3.1-alpha", "a-v1.3.1-beta"}}, }, }, { @@ -235,7 +368,7 @@ func TestGenerateChannels(t *testing.T) { Entries: []declcfg.ChannelEntry{ {Name: "a-v1.4.1-beta1", Replaces: ""}, {Name: "a-v1.4.1-beta2", Replaces: ""}, - {Name: "a-v1.4.1", Replaces: "a-v1.3.1", Skips: []string{"a-v1.1.0", "a-v1.2.1", "a-v1.3.1-alpha", "a-v1.3.1-beta", "a-v1.4.1-beta1", "a-v1.4.1-beta2"}}, + {Name: "a-v1.4.1", Replaces: "", Skips: []string{"a-v1.4.1-beta1", "a-v1.4.1-beta2"}}, }, }, { @@ -253,7 +386,7 @@ func TestGenerateChannels(t *testing.T) { Package: "a", Entries: []declcfg.ChannelEntry{ {Name: "a-v2.3.1", Replaces: ""}, - {Name: "a-v2.3.2", Replaces: "a-v2.1.1", Skips: []string{"a-v2.1.0", "a-v2.3.1"}}, + {Name: "a-v2.3.2", Replaces: "", Skips: []string{"a-v2.3.1"}}, }, }, { @@ -332,7 +465,8 @@ func TestGenerateChannels(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { sv := &semverTemplate{GenerateMajorChannels: tt.generateMajorChannels, GenerateMinorChannels: tt.generateMinorChannels, pkg: "a", DefaultChannelTypePreference: tt.channelTypePreference} - require.ElementsMatch(t, tt.out, sv.generateChannels(&channelOperatorVersions)) + out := sv.generateChannels(&channelOperatorVersions) + require.ElementsMatch(t, tt.out, out) require.Equal(t, tt.defaultChannel, sv.defaultChannel) }) } diff --git a/alpha/template/semver/types.go b/alpha/template/semver/types.go index de68504c6..971718b34 100644 --- a/alpha/template/semver/types.go +++ b/alpha/template/semver/types.go @@ -1,7 +1,6 @@ package semver import ( - "fmt" "io" "github.com/blang/semver/v4" @@ -81,16 +80,3 @@ type highwaterChannel struct { version semver.Version name string } - -type entryTuple struct { - arch channelArchetype - kind streamType - name string - parent string - index int - version semver.Version -} - -func (t entryTuple) String() string { - return fmt.Sprintf("{ arch: %q, kind: %q, name: %q, parent: %q, index: %d, version: %v }", t.arch, t.kind, t.name, t.parent, t.index, t.version.String()) -}