From c404b49f69c72b2ce8a7620c952c52c2d4b03055 Mon Sep 17 00:00:00 2001 From: Vu Dinh Date: Fri, 29 Apr 2022 10:33:13 +0700 Subject: [PATCH] feat(diff): change HeadsOnly mode to be an explicit setting Currently the headsOnly mode is implilitly set if newRef is not provided. This behavior is unclear and leads to confusion. This commit will change the current headsOnly to be explicit setting that will be converted to package setting that will be executed accordingly. Signed-off-by: Vu Dinh --- alpha/action/diff.go | 3 + alpha/action/diff_test.go | 15 +++- alpha/declcfg/diff.go | 9 ++- alpha/declcfg/diff_include.go | 35 +++++++-- alpha/declcfg/diff_test.go | 140 +++++++++++++++++++++++++++++++++- cmd/opm/alpha/diff/cmd.go | 4 + 6 files changed, 192 insertions(+), 14 deletions(-) diff --git a/alpha/action/diff.go b/alpha/action/diff.go index 42960b3cd..184428f13 100644 --- a/alpha/action/diff.go +++ b/alpha/action/diff.go @@ -28,6 +28,8 @@ type Diff struct { IncludeConfig DiffIncludeConfig // IncludeAdditively catalog objects specified in IncludeConfig. IncludeAdditively bool + // HeadsOnly is the mode that selects the head of the channels only. + HeadsOnly bool Logger *logrus.Entry } @@ -75,6 +77,7 @@ func (diff Diff) Run(ctx context.Context) (*declcfg.DeclarativeConfig, error) { SkipDependencies: diff.SkipDependencies, Includer: convertIncludeConfigToIncluder(diff.IncludeConfig), IncludeAdditively: diff.IncludeAdditively, + HeadsOnly: diff.HeadsOnly, } diffModel, err := g.Run(oldModel, newModel) if err != nil { diff --git a/alpha/action/diff_test.go b/alpha/action/diff_test.go index a67ec32c0..e3903b616 100644 --- a/alpha/action/diff_test.go +++ b/alpha/action/diff_test.go @@ -46,8 +46,9 @@ func TestDiff(t *testing.T) { { name: "Success/HeadsOnly", diff: Diff{ - Registry: registry, - NewRefs: []string{filepath.Join("testdata", "index-declcfgs", "latest")}, + Registry: registry, + NewRefs: []string{filepath.Join("testdata", "index-declcfgs", "latest")}, + HeadsOnly: true, }, expectedCfg: loadDirFS(t, indicesDir, filepath.Join("testdata", "index-declcfgs", "exp-headsonly")), assertion: require.NoError, @@ -61,6 +62,7 @@ func TestDiff(t *testing.T) { Packages: []DiffIncludePackage{{Name: "baz"}}, }, IncludeAdditively: true, + HeadsOnly: true, }, expectedCfg: loadDirFS(t, indicesDir, filepath.Join("testdata", "index-declcfgs", "exp-include-pkg")), assertion: require.NoError, @@ -79,6 +81,7 @@ func TestDiff(t *testing.T) { }, }, IncludeAdditively: true, + HeadsOnly: true, }, expectedCfg: loadDirFS(t, indicesDir, filepath.Join("testdata", "index-declcfgs", "exp-include-channel")), assertion: require.NoError, @@ -97,6 +100,7 @@ func TestDiff(t *testing.T) { }, }, IncludeAdditively: true, + HeadsOnly: true, }, expectedCfg: loadDirFS(t, indicesDir, filepath.Join("testdata", "index-declcfgs", "exp-include-channel")), assertion: require.NoError, @@ -115,6 +119,7 @@ func TestDiff(t *testing.T) { }, }, IncludeAdditively: true, + HeadsOnly: true, }, expectedCfg: loadDirFS(t, indicesDir, filepath.Join("testdata", "index-declcfgs", "exp-include-channel")), assertion: require.NoError, @@ -134,6 +139,7 @@ func TestDiff(t *testing.T) { }, }, IncludeAdditively: true, + HeadsOnly: true, }, expectedCfg: loadDirFS(t, indicesDir, filepath.Join("testdata", "index-declcfgs", "exp-include-channel")), assertion: require.NoError, @@ -141,8 +147,9 @@ func TestDiff(t *testing.T) { { name: "Fail/NewBundleImage", diff: Diff{ - Registry: registry, - NewRefs: []string{"test.registry/foo-operator/foo-bundle:v0.1.0"}, + Registry: registry, + NewRefs: []string{"test.registry/foo-operator/foo-bundle:v0.1.0"}, + HeadsOnly: true, }, assertion: func(t require.TestingT, err error, _ ...interface{}) { if !assert.Error(t, err) { diff --git a/alpha/declcfg/diff.go b/alpha/declcfg/diff.go index 728586a2a..fa1d4ce02 100644 --- a/alpha/declcfg/diff.go +++ b/alpha/declcfg/diff.go @@ -25,6 +25,8 @@ type DiffGenerator struct { Includer DiffIncluder // IncludeAdditively catalog objects specified in Includer in headsOnly mode. IncludeAdditively bool + // HeadsOnly is the mode that selects the head of the channels only. + HeadsOnly bool initOnce sync.Once } @@ -37,6 +39,8 @@ func (g *DiffGenerator) init() { if g.Includer.Logger == nil { g.Includer.Logger = g.Logger } + // Inject headsOnly setting into DiffIncluder from command line setting + g.Includer.HeadsOnly = g.HeadsOnly }) } @@ -79,7 +83,7 @@ func (g *DiffGenerator) Run(oldModel, newModel model.Model) (model.Model, error) return nil } - headsOnlyMode := len(oldModel) == 0 + headsOnlyMode := g.HeadsOnly latestMode := !headsOnlyMode isInclude := len(g.Includer.Packages) != 0 @@ -111,6 +115,9 @@ func (g *DiffGenerator) Run(oldModel, newModel model.Model) (model.Model, error) case isInclude: // Add included objects to outputModel. + // Assume heads-only is false for include additively since we already have the channel heads + // in the output model. + g.Includer.HeadsOnly = false // Add included packages/channels/bundles from newModel to outputModel. if err := g.Includer.Run(newModel, outputModel); err != nil { return nil, err diff --git a/alpha/declcfg/diff_include.go b/alpha/declcfg/diff_include.go index 2855702b1..d92f76877 100644 --- a/alpha/declcfg/diff_include.go +++ b/alpha/declcfg/diff_include.go @@ -17,6 +17,9 @@ type DiffIncluder struct { // Packages to add. Packages []DiffIncludePackage Logger *logrus.Entry + // HeadsOnly is the mode that selects the head of the channels only. + // This setting will be overridden by any versions or bundles in the channels. + HeadsOnly bool } // DiffIncludePackage specifies a package, and optionally channels @@ -35,6 +38,9 @@ type DiffIncludePackage struct { // Package range setting is mutually exclusive with channel range/bundles/version // settings. Range semver.Range + // HeadsOnly is the mode that selects the head of the channels only. + // This setting will be overridden by any versions or bundles in the channels. + HeadsOnly bool } // DiffIncludeChannel specifies a channel, and optionally bundles and bundle versions @@ -126,6 +132,7 @@ func (i DiffIncluder) Run(newModel, outputModel model.Model) error { for _, ipkg := range i.Packages { pkgLog := i.Logger.WithField("package", ipkg.Name) + ipkg.HeadsOnly = i.HeadsOnly includeErrs = append(includeErrs, ipkg.includeNewInOutputModel(newModel, outputModel, pkgLog)...) } if len(includeErrs) != 0 { @@ -146,10 +153,20 @@ func (ipkg DiffIncludePackage) includeNewInOutputModel(newModel, outputModel mod } pkgLog := logger.WithField("package", newPkg.Name) - // No channels or versions were specified, meaning "include the full package". + // No range, channels or versions were specified if len(ipkg.Channels) == 0 && len(ipkg.AllChannels.Versions) == 0 && len(ipkg.AllChannels.Bundles) == 0 && ipkg.Range == nil { - outputModel[ipkg.Name] = newPkg - return nil + // heads-only false, meaning "include the full package". + if !ipkg.HeadsOnly { + outputModel[ipkg.Name] = newPkg + return nil + } + // heads-only true, get the head of every channel in the package + for _, c := range newPkg.Channels { + newCh := DiffIncludeChannel{ + Name: c.Name, + } + ipkg.Channels = append(ipkg.Channels, newCh) + } } outputPkg := copyPackageNoChannels(newPkg) @@ -197,16 +214,24 @@ func (ipkg DiffIncludePackage) includeNewInOutputModel(newModel, outputModel mod chLog := pkgLog.WithField("channel", newCh.Name) var bundles []*model.Bundle + var head *model.Bundle var err error - if ich.Range != nil { + // No versions have been specified, but heads-only set to true, get the channel head only. + switch { + case ipkg.HeadsOnly && len(ich.Versions) == 0 && len(ich.Bundles) == 0 && ich.Range == nil: + head, err = newCh.Head() + bundles = append(bundles, head) + case ich.Range != nil: bundles, err = getBundlesForRange(newCh, ich.Range, chLog) - } else { + default: bundles, err = getBundlesForVersions(newCh, ich.Versions, ich.Bundles, chLog, skipMissingBundleForChannels[newCh.Name]) } + if err != nil { ierrs = append(ierrs, fmt.Errorf("[package=%q channel=%q] %v", newPkg.Name, newCh.Name, err)) continue } + outputCh := copyChannelNoBundles(newCh, outputPkg) outputPkg.Channels[outputCh.Name] = outputCh for _, b := range bundles { diff --git a/alpha/declcfg/diff_test.go b/alpha/declcfg/diff_test.go index 4729214e2..9aad434a4 100644 --- a/alpha/declcfg/diff_test.go +++ b/alpha/declcfg/diff_test.go @@ -1318,6 +1318,7 @@ func TestDiffHeadsOnly(t *testing.T) { }, g: &DiffGenerator{ IncludeAdditively: false, + HeadsOnly: true, Includer: DiffIncluder{ Packages: []DiffIncludePackage{ { @@ -1354,7 +1355,9 @@ func TestDiffHeadsOnly(t *testing.T) { }, }, }, - g: &DiffGenerator{}, + g: &DiffGenerator{ + HeadsOnly: true, + }, expCfg: DeclarativeConfig{ Packages: []Package{ {Schema: schemaPackage, Name: "foo", DefaultChannel: "stable"}, @@ -1468,7 +1471,9 @@ func TestDiffHeadsOnly(t *testing.T) { }, }, }, - g: &DiffGenerator{}, + g: &DiffGenerator{ + HeadsOnly: true, + }, expCfg: DeclarativeConfig{ Packages: []Package{ {Schema: schemaPackage, Name: "etcd", DefaultChannel: "stable"}, @@ -1559,7 +1564,9 @@ func TestDiffHeadsOnly(t *testing.T) { }, }, }, - g: &DiffGenerator{}, + g: &DiffGenerator{ + HeadsOnly: true, + }, expCfg: DeclarativeConfig{ Packages: []Package{ {Schema: schemaPackage, Name: "bar", DefaultChannel: "stable"}, @@ -1652,6 +1659,7 @@ func TestDiffHeadsOnly(t *testing.T) { }, g: &DiffGenerator{ SkipDependencies: true, + HeadsOnly: true, }, expCfg: DeclarativeConfig{ Packages: []Package{ @@ -1788,7 +1796,9 @@ func TestDiffHeadsOnly(t *testing.T) { }, }, }, - g: &DiffGenerator{}, + g: &DiffGenerator{ + HeadsOnly: true, + }, expCfg: DeclarativeConfig{ Packages: []Package{ {Schema: schemaPackage, Name: "bar", DefaultChannel: "stable"}, @@ -1980,6 +1990,7 @@ func TestDiffHeadsOnly(t *testing.T) { }, g: &DiffGenerator{ IncludeAdditively: false, + HeadsOnly: true, Includer: DiffIncluder{ Packages: []DiffIncludePackage{ { @@ -2127,6 +2138,7 @@ func TestDiffHeadsOnly(t *testing.T) { }, g: &DiffGenerator{ IncludeAdditively: true, + HeadsOnly: true, Includer: DiffIncluder{ Packages: []DiffIncludePackage{ { @@ -2263,6 +2275,7 @@ func TestDiffHeadsOnly(t *testing.T) { Includer: DiffIncluder{ Packages: []DiffIncludePackage{{Name: "bar"}}, }, + HeadsOnly: false, }, expCfg: DeclarativeConfig{ Packages: []Package{ @@ -2328,6 +2341,7 @@ func TestDiffHeadsOnly(t *testing.T) { Includer: DiffIncluder{ Packages: []DiffIncludePackage{{Name: "foo", Channels: []DiffIncludeChannel{{Name: "stable"}}}}, }, + HeadsOnly: false, }, expCfg: DeclarativeConfig{ Packages: []Package{ @@ -2352,6 +2366,122 @@ func TestDiffHeadsOnly(t *testing.T) { }, }, }, + { + name: "HasDiff/IncludePackageHeadsOnly", + newCfg: DeclarativeConfig{ + Packages: []Package{ + {Schema: schemaPackage, Name: "foo", DefaultChannel: "stable"}, + {Schema: schemaPackage, Name: "bar", DefaultChannel: "stable"}, + }, + Channels: []Channel{ + {Schema: schemaChannel, Name: "stable", Package: "foo", Entries: []ChannelEntry{{Name: "foo.v0.1.0"}}}, + {Schema: schemaChannel, Name: "stable", Package: "bar", Entries: []ChannelEntry{ + {Name: "bar.v0.1.0"}, {Name: "bar.v0.2.0", Replaces: "bar.v0.1.0"}, + }}, + }, + Bundles: []Bundle{ + { + Schema: schemaBundle, + Name: "foo.v0.1.0", Package: "foo", Image: "reg/foo:latest", + Properties: []property.Property{property.MustBuildPackage("foo", "0.1.0")}, + }, + { + Schema: schemaBundle, + Name: "bar.v0.1.0", Package: "bar", Image: "reg/bar:latest", + Properties: []property.Property{property.MustBuildPackage("bar", "0.1.0")}, + }, + { + Schema: schemaBundle, + Name: "bar.v0.2.0", Package: "bar", Image: "reg/bar:latest", + Properties: []property.Property{property.MustBuildPackage("bar", "0.2.0")}, + }, + }, + }, + g: &DiffGenerator{ + Includer: DiffIncluder{ + Packages: []DiffIncludePackage{{Name: "bar"}}, + }, + HeadsOnly: true, + }, + expCfg: DeclarativeConfig{ + Packages: []Package{ + {Schema: schemaPackage, Name: "bar", DefaultChannel: "stable"}, + }, + Channels: []Channel{ + {Schema: schemaChannel, Name: "stable", Package: "bar", Entries: []ChannelEntry{ + {Name: "bar.v0.2.0", Replaces: "bar.v0.1.0"}, + }}, + }, + Bundles: []Bundle{ + { + Schema: schemaBundle, + Name: "bar.v0.2.0", Package: "bar", Image: "reg/bar:latest", + Properties: []property.Property{property.MustBuildPackage("bar", "0.2.0")}, + }, + }, + }, + }, + { + name: "HasDiff/IncludeChannelHeadsOnly", + newCfg: DeclarativeConfig{ + Packages: []Package{ + {Schema: schemaPackage, Name: "foo", DefaultChannel: "alpha"}, // Make sure the default channel is still updated. + }, + Channels: []Channel{ + {Schema: schemaChannel, Name: "stable", Package: "foo", Entries: []ChannelEntry{ + {Name: "foo.v0.1.0"}, {Name: "foo.v0.2.0", Replaces: "foo.v0.1.0"}}, + }, + {Schema: schemaChannel, Name: "alpha", Package: "foo", Entries: []ChannelEntry{ + {Name: "foo.v0.1.0-alpha.0"}, {Name: "foo.v0.2.0-alpha.0", Replaces: "foo.v0.1.0-alpha.0"}}, + }, + }, + Bundles: []Bundle{ + { + Schema: schemaBundle, + Name: "foo.v0.1.0", Package: "foo", Image: "reg/foo:latest", + Properties: []property.Property{property.MustBuildPackage("foo", "0.1.0")}, + }, + { + Schema: schemaBundle, + Name: "foo.v0.2.0", Package: "foo", Image: "reg/foo:latest", + Properties: []property.Property{property.MustBuildPackage("foo", "0.2.0")}, + }, + { + Schema: schemaBundle, + Name: "foo.v0.1.0-alpha.0", Package: "foo", Image: "reg/foo:latest", + Properties: []property.Property{property.MustBuildPackage("foo", "0.1.0-alpha.0")}, + }, + { + Schema: schemaBundle, + Name: "foo.v0.2.0-alpha.0", Package: "foo", Image: "reg/foo:latest", + Properties: []property.Property{property.MustBuildPackage("foo", "0.2.0-alpha.0")}, + }, + }, + }, + g: &DiffGenerator{ + Includer: DiffIncluder{ + Packages: []DiffIncludePackage{{Name: "foo", Channels: []DiffIncludeChannel{{Name: "stable"}}}}, + }, + HeadsOnly: true, + }, + expCfg: DeclarativeConfig{ + Packages: []Package{ + {Schema: schemaPackage, Name: "foo", DefaultChannel: "alpha"}, + }, + Channels: []Channel{ + {Schema: schemaChannel, Name: "stable", Package: "foo", Entries: []ChannelEntry{ + {Name: "foo.v0.2.0", Replaces: "foo.v0.1.0"}}, + }, + }, + Bundles: []Bundle{ + { + Schema: schemaBundle, + Name: "foo.v0.2.0", Package: "foo", Image: "reg/foo:latest", + Properties: []property.Property{property.MustBuildPackage("foo", "0.2.0")}, + }, + }, + }, + }, { name: "HasDiff/IncludeVersion", newCfg: DeclarativeConfig{ @@ -2394,6 +2524,7 @@ func TestDiffHeadsOnly(t *testing.T) { {Name: "stable", Versions: []semver.Version{{Major: 0, Minor: 2, Patch: 0}}}}, }}, }, + HeadsOnly: true, }, expCfg: DeclarativeConfig{ Packages: []Package{ @@ -2531,6 +2662,7 @@ func TestDiffHeadsOnly(t *testing.T) { }, }, }, + HeadsOnly: true, }, expCfg: DeclarativeConfig{ Packages: []Package{ diff --git a/cmd/opm/alpha/diff/cmd.go b/cmd/opm/alpha/diff/cmd.go index 02cd52b28..cbc2a96b1 100644 --- a/cmd/opm/alpha/diff/cmd.go +++ b/cmd/opm/alpha/diff/cmd.go @@ -29,6 +29,7 @@ type input struct { newRefs []string skipDeps bool includeAdditive bool + headsOnly bool includeFile string output string @@ -133,6 +134,8 @@ docker push registry.org/my-catalog:diff-latest "Upgrade graphs from individual bundles/versions to their channel's head are also included") cmd.Flags().BoolVar(&in.includeAdditive, "include-additive", false, "Ref objects from --include-file are returned on top of 'heads-only' or 'latest' output") + cmd.Flags().BoolVar(&in.headsOnly, "headsOnly", false, + "Using `headsOnly` mode where head(s) of the channel(s) in the package are selected") cmd.Flags().BoolVar(&in.debug, "debug", false, "enable debug logging") return cmd @@ -185,6 +188,7 @@ func (in *input) diffFunc(cmd *cobra.Command, args []string) error { NewRefs: in.newRefs, SkipDependencies: in.skipDeps, IncludeAdditively: in.includeAdditive, + HeadsOnly: in.headsOnly, Logger: in.logger, }