From 6ad003f8e6e4543e718b8d12c3202ba971d110a9 Mon Sep 17 00:00:00 2001 From: Eric Stroczynski Date: Fri, 5 Nov 2021 13:55:12 -0700 Subject: [PATCH 1/4] feat(diff): introduce MergeTypes to merge objects with the same unique key Signed-off-by: Eric Stroczynski --- alpha/action/diff.go | 8 + alpha/action/merge.go | 260 +++++++++++++++++++++++ alpha/action/merge_test.go | 407 +++++++++++++++++++++++++++++++++++++ cmd/opm/alpha/diff/cmd.go | 2 + go.mod | 1 + vendor/modules.txt | 1 + 6 files changed, 679 insertions(+) create mode 100644 alpha/action/merge.go create mode 100644 alpha/action/merge_test.go diff --git a/alpha/action/diff.go b/alpha/action/diff.go index 7912cf8a7..9be68d703 100644 --- a/alpha/action/diff.go +++ b/alpha/action/diff.go @@ -29,6 +29,8 @@ type Diff struct { // IncludeAdditively catalog objects specified in IncludeConfig. IncludeAdditively bool + MergeType MergeType + Logger *logrus.Entry } @@ -51,6 +53,9 @@ func (a Diff) Run(ctx context.Context) (*declcfg.DeclarativeConfig, error) { } return nil, fmt.Errorf("error rendering old refs: %v", err) } + if err := a.MergeType.mergeDC(oldCfg); err != nil { + return nil, fmt.Errorf("error merging across old refs: %v", err) + } oldModel, err = declcfg.ConvertToModel(*oldCfg) if err != nil { return nil, fmt.Errorf("error converting old declarative config to model: %v", err) @@ -65,6 +70,9 @@ func (a Diff) Run(ctx context.Context) (*declcfg.DeclarativeConfig, error) { } return nil, fmt.Errorf("error rendering new refs: %v", err) } + if err := a.MergeType.mergeDC(newCfg); err != nil { + return nil, fmt.Errorf("error merging across new refs: %v", err) + } newModel, err := declcfg.ConvertToModel(*newCfg) if err != nil { return nil, fmt.Errorf("error converting new declarative config to model: %v", err) diff --git a/alpha/action/merge.go b/alpha/action/merge.go new file mode 100644 index 000000000..0fd5e3344 --- /dev/null +++ b/alpha/action/merge.go @@ -0,0 +1,260 @@ +package action + +import ( + "fmt" + "sort" + + "github.com/imdario/mergo" + + "github.com/operator-framework/operator-registry/alpha/declcfg" +) + +// MergeType is the merge strategy used to combine declarative config objects +// that have the same unique key. +type MergeType int + +const ( + // PreferLast just uses the last/most recent object. + // This is the default merge type. + PreferLast MergeType = iota + // TwoWay fills in object fields by combining objects in ascending order, + // starting with the first/oldest. + // Bundle properties are NOT merged because an arbitrary property's key is unknowable. + TwoWay +) + +func (mt MergeType) mergeDC(cfg *declcfg.DeclarativeConfig) error { + switch mt { + case PreferLast: + return mergeDCPreferLast(cfg) + case TwoWay: + return mergeDCTwoWay(cfg) + default: + return fmt.Errorf("unknown merge type %v", mt) + } +} + +func keyForDCObj(obj interface{}) string { + switch t := obj.(type) { + case declcfg.Package: + // Package name is globally unique. + return t.Name + case declcfg.Channel: + // Channel name is unqiue per package. + return t.Package + t.Name + case declcfg.Bundle: + // Bundle name is unqiue per package. + return t.Package + t.Name + default: + // This should never happen. + panic(fmt.Sprintf("bug: unrecognized type %T, expected one of Package, Channel, Bundle", t)) + } +} + +// mergeDCPreferLast merges all packages, channels, and bundles with the same unique key +// into single objects using the last element with that key. +func mergeDCPreferLast(cfg *declcfg.DeclarativeConfig) error { + + // Merge packages. + pkgsByKey := make(map[string][]declcfg.Package, len(cfg.Packages)) + for i, pkg := range cfg.Packages { + key := keyForDCObj(pkg) + pkgsByKey[key] = append(pkgsByKey[key], cfg.Packages[i]) + } + if len(pkgsByKey) != 0 { + outPkgs := make([]declcfg.Package, len(pkgsByKey)) + i := 0 + for _, pkgs := range pkgsByKey { + outPkgs[i] = pkgs[len(pkgs)-1] + i++ + } + sortPackages(outPkgs) + cfg.Packages = outPkgs + } + + // Merge channels. + chsByKey := make(map[string][]declcfg.Channel, len(cfg.Channels)) + for i, ch := range cfg.Channels { + key := keyForDCObj(ch) + chsByKey[key] = append(chsByKey[key], cfg.Channels[i]) + } + if len(chsByKey) != 0 { + outChs := make([]declcfg.Channel, len(chsByKey)) + i := 0 + for _, chs := range chsByKey { + outChs[i] = chs[len(chs)-1] + i++ + } + sortChannels(outChs) + cfg.Channels = outChs + } + + // Merge bundles. + bundlesByKey := make(map[string][]declcfg.Bundle, len(cfg.Bundles)) + for i, b := range cfg.Bundles { + key := keyForDCObj(b) + bundlesByKey[key] = append(bundlesByKey[key], cfg.Bundles[i]) + } + if len(bundlesByKey) != 0 { + outBundles := make([]declcfg.Bundle, len(bundlesByKey)) + i := 0 + for _, bundles := range bundlesByKey { + outBundles[i] = bundles[len(bundles)-1] + i++ + } + sortBundles(outBundles) + cfg.Bundles = outBundles + } + + // There is no way to merge "other" schema since a unique key field is unknown. + return nil +} + +// mergeDCTwoWay merges all packages, channels, and bundles with the same unique key +// into single objects with ascending priority. +func mergeDCTwoWay(cfg *declcfg.DeclarativeConfig) error { + var err error + if cfg.Packages, err = mergePackages(cfg.Packages); err != nil { + return err + } + if cfg.Channels, err = mergeChannels(cfg.Channels); err != nil { + return err + } + if cfg.Bundles, err = mergeBundles(cfg.Bundles); err != nil { + return err + } + // There is no way to merge "other" schema since a unique key field is unknown. + return nil +} + +// mergePackages merges all packages with the same name into one package object. +// Value preference is ascending: values of packages later in input are preferred. +func mergePackages(inPkgs []declcfg.Package) (outPkgs []declcfg.Package, err error) { + pkgsByName := make(map[string][]declcfg.Package, len(inPkgs)) + for i, pkg := range inPkgs { + key := keyForDCObj(pkg) + pkgsByName[key] = append(pkgsByName[key], inPkgs[i]) + } + + for _, pkgs := range pkgsByName { + mergedPkg := pkgs[0] + + if len(pkgs) > 1 { + for _, pkg := range pkgs[1:] { + if err := mergo.Merge(&mergedPkg, pkg, mergo.WithOverride); err != nil { + return nil, err + } + } + } + + outPkgs = append(outPkgs, mergedPkg) + } + + sortPackages(outPkgs) + + return outPkgs, nil +} + +// mergeChannels merges all channels with the same name and package into one channel object. +// Value preference is ascending: values of channels later in input are preferred. +func mergeChannels(inChs []declcfg.Channel) (outChs []declcfg.Channel, err error) { + chsByKey := make(map[string][]declcfg.Channel, len(inChs)) + entriesByKey := make(map[string]map[string][]declcfg.ChannelEntry, len(inChs)) + for i, ch := range inChs { + chKey := keyForDCObj(ch) + chsByKey[chKey] = append(chsByKey[chKey], inChs[i]) + entriesByKey[chKey] = make(map[string][]declcfg.ChannelEntry) + for j, e := range ch.Entries { + entriesByKey[chKey][e.Name] = append(entriesByKey[chKey][e.Name], ch.Entries[j]) + } + } + + for chKey, chs := range chsByKey { + mergedCh := chs[0] + + if len(chs) > 1 { + for _, ch := range chs[1:] { + if err := mergo.Merge(&mergedCh, ch, mergo.WithOverride); err != nil { + return nil, err + } + } + } + + mergedCh.Entries = nil + for _, entries := range entriesByKey[chKey] { + mergedEntry := entries[0] + + if len(entries) > 1 { + for _, e := range entries[1:] { + if err := mergo.Merge(&mergedEntry, e, mergo.WithOverride); err != nil { + return nil, err + } + } + } + + mergedCh.Entries = append(mergedCh.Entries, mergedEntry) + } + + sort.Slice(mergedCh.Entries, func(i, j int) bool { + return mergedCh.Entries[i].Name < mergedCh.Entries[j].Name + }) + + outChs = append(outChs, mergedCh) + } + + sortChannels(outChs) + + return outChs, nil +} + +// mergeBundles merges all bundles with the same name and package into one bundle object. +// Value preference is ascending: values of bundles later in input are preferred. +func mergeBundles(inBundles []declcfg.Bundle) (outBundles []declcfg.Bundle, err error) { + bundlesByKey := make(map[string][]declcfg.Bundle, len(inBundles)) + for i, bundle := range inBundles { + key := keyForDCObj(bundle) + bundlesByKey[key] = append(bundlesByKey[key], inBundles[i]) + } + + for _, bundles := range bundlesByKey { + mergedBundle := bundles[0] + + if len(bundles) > 1 { + for _, bundle := range bundles[1:] { + if err := mergo.Merge(&mergedBundle, bundle, mergo.WithOverride); err != nil { + return nil, err + } + } + } + + outBundles = append(outBundles, mergedBundle) + } + + sortBundles(outBundles) + + return outBundles, nil +} + +func sortPackages(pkgs []declcfg.Package) { + sort.Slice(pkgs, func(i, j int) bool { + return pkgs[i].Name < pkgs[j].Name + }) +} + +func sortChannels(chs []declcfg.Channel) { + sort.Slice(chs, func(i, j int) bool { + if chs[i].Package == chs[j].Package { + return chs[i].Name < chs[j].Name + } + return chs[i].Package < chs[j].Package + }) +} + +func sortBundles(bundles []declcfg.Bundle) { + sort.Slice(bundles, func(i, j int) bool { + if bundles[i].Package == bundles[j].Package { + return bundles[i].Name < bundles[j].Name + } + return bundles[i].Package < bundles[j].Package + }) +} diff --git a/alpha/action/merge_test.go b/alpha/action/merge_test.go new file mode 100644 index 000000000..20babedc2 --- /dev/null +++ b/alpha/action/merge_test.go @@ -0,0 +1,407 @@ +package action + +import ( + "testing" + + "github.com/operator-framework/operator-registry/alpha/declcfg" + "github.com/operator-framework/operator-registry/alpha/property" + "github.com/stretchr/testify/require" +) + +func TestMergeDC(t *testing.T) { + type spec struct { + name string + mt MergeType + dc, expDC *declcfg.DeclarativeConfig + expError string + } + + cases := []spec{ + { + name: "TwoWay/Empty", + mt: TwoWay, + dc: &declcfg.DeclarativeConfig{}, + expDC: &declcfg.DeclarativeConfig{}, + }, + { + name: "TwoWay/NoMergeNeeded", + mt: TwoWay, + dc: &declcfg.DeclarativeConfig{ + Packages: []declcfg.Package{ + {Schema: "olm.package", Name: "foo", DefaultChannel: "stable"}, + {Schema: "olm.package", Name: "bar", DefaultChannel: "stable"}, + }, + Channels: []declcfg.Channel{ + {Schema: "olm.channel", Name: "stable", Package: "foo", Entries: []declcfg.ChannelEntry{ + {Name: "foo.v0.1.0"}, + }}, + {Schema: "olm.channel", Name: "stable", Package: "bar", Entries: []declcfg.ChannelEntry{ + {Name: "bar.v0.1.0"}, + }}, + }, + Bundles: []declcfg.Bundle{ + { + Schema: "olm.bundle", + Name: "foo.v0.1.0", + Package: "foo", + Image: "reg/foo:latest", + Properties: []property.Property{ + property.MustBuildPackage("foo", "0.1.0"), + }, + }, + { + Schema: "olm.bundle", + Name: "bar.v0.1.0", + Package: "bar", + Image: "reg/bar:latest", + Properties: []property.Property{ + property.MustBuildGVKRequired("etcd.database.coreos.com", "v1", "EtcdBackup"), + property.MustBuildPackage("bar", "0.1.0"), + }, + }, + }, + }, + expDC: &declcfg.DeclarativeConfig{ + Packages: []declcfg.Package{ + {Schema: "olm.package", Name: "bar", DefaultChannel: "stable"}, + {Schema: "olm.package", Name: "foo", DefaultChannel: "stable"}, + }, + Channels: []declcfg.Channel{ + {Schema: "olm.channel", Name: "stable", Package: "bar", Entries: []declcfg.ChannelEntry{ + {Name: "bar.v0.1.0"}, + }}, + {Schema: "olm.channel", Name: "stable", Package: "foo", Entries: []declcfg.ChannelEntry{ + {Name: "foo.v0.1.0"}, + }}, + }, + Bundles: []declcfg.Bundle{ + { + Schema: "olm.bundle", + Name: "bar.v0.1.0", + Package: "bar", + Image: "reg/bar:latest", + Properties: []property.Property{ + property.MustBuildGVKRequired("etcd.database.coreos.com", "v1", "EtcdBackup"), + property.MustBuildPackage("bar", "0.1.0"), + }, + }, + { + Schema: "olm.bundle", + Name: "foo.v0.1.0", + Package: "foo", + Image: "reg/foo:latest", + Properties: []property.Property{ + property.MustBuildPackage("foo", "0.1.0"), + }, + }, + }, + }, + }, + { + name: "TwoWay/MergePackagesChannelsBundles", + mt: TwoWay, + dc: &declcfg.DeclarativeConfig{ + Packages: []declcfg.Package{ + {Schema: "olm.package", Name: "foo", DefaultChannel: "stable"}, + {Schema: "olm.package", Name: "foo", DefaultChannel: "alpha"}, + {Schema: "olm.package", Name: "bar", DefaultChannel: "stable"}, + }, + Channels: []declcfg.Channel{ + {Schema: "olm.channel", Name: "stable", Package: "foo", Entries: []declcfg.ChannelEntry{ + {Name: "foo.v0.1.0"}, + {Name: "foo.v0.1.0", Replaces: "foo.v0.0.5"}, + {Name: "foo.v0.1.0", Skips: []string{"foo.v0.0.4"}}, + }}, + {Schema: "olm.channel", Name: "stable", Package: "bar", Entries: []declcfg.ChannelEntry{ + {Name: "bar.v0.1.0"}, + }}, + }, + Bundles: []declcfg.Bundle{ + { + Schema: "olm.bundle", + Name: "foo.v0.0.5", + Package: "foo", + Image: "reg/foo:latest", + Properties: []property.Property{ + property.MustBuildPackage("foo", "0.0.5"), + }, + }, + { + Schema: "olm.bundle", + Name: "foo.v0.1.0", + Package: "foo", + Image: "reg/foo:latest", + Properties: []property.Property{ + property.MustBuildPackage("foo", "0.1.0"), + }, + }, + { + Schema: "olm.bundle", + Name: "bar.v0.1.0", + Package: "bar", + Image: "reg/bar:latest", + Properties: []property.Property{ + property.MustBuildGVKRequired("foo.example.com", "v1", "Foo"), + property.MustBuildPackage("bar", "0.1.0"), + }, + }, + { + Schema: "olm.bundle", + Name: "bar.v0.1.0", + Package: "bar", + Image: "reg/bar:latest", + Properties: []property.Property{ + property.MustBuildGVKRequired("etcd.database.coreos.com", "v1", "EtcdBackup"), + property.MustBuildPackage("bar", "0.1.0"), + }, + }, + }, + }, + expDC: &declcfg.DeclarativeConfig{ + Packages: []declcfg.Package{ + {Schema: "olm.package", Name: "bar", DefaultChannel: "stable"}, + {Schema: "olm.package", Name: "foo", DefaultChannel: "alpha"}, + }, + Channels: []declcfg.Channel{ + {Schema: "olm.channel", Name: "stable", Package: "bar", Entries: []declcfg.ChannelEntry{ + {Name: "bar.v0.1.0"}, + }}, + {Schema: "olm.channel", Name: "stable", Package: "foo", Entries: []declcfg.ChannelEntry{ + {Name: "foo.v0.1.0", Replaces: "foo.v0.0.5", Skips: []string{"foo.v0.0.4"}}, + }}, + }, + Bundles: []declcfg.Bundle{ + { + Schema: "olm.bundle", + Name: "bar.v0.1.0", + Package: "bar", + Image: "reg/bar:latest", + Properties: []property.Property{ + // Can't merge properties since their keys are unknown. + property.MustBuildGVKRequired("etcd.database.coreos.com", "v1", "EtcdBackup"), + property.MustBuildPackage("bar", "0.1.0"), + }, + }, + { + Schema: "olm.bundle", + Name: "foo.v0.0.5", + Package: "foo", + Image: "reg/foo:latest", + Properties: []property.Property{ + property.MustBuildPackage("foo", "0.0.5"), + }, + }, + { + Schema: "olm.bundle", + Name: "foo.v0.1.0", + Package: "foo", + Image: "reg/foo:latest", + Properties: []property.Property{ + property.MustBuildPackage("foo", "0.1.0"), + }, + }, + }, + }, + }, + { + name: "PreferLast/Empty", + mt: PreferLast, + dc: &declcfg.DeclarativeConfig{}, + expDC: &declcfg.DeclarativeConfig{}, + }, + { + name: "PreferLast/NoMergeNeeded", + mt: PreferLast, + dc: &declcfg.DeclarativeConfig{ + Packages: []declcfg.Package{ + {Schema: "olm.package", Name: "foo", DefaultChannel: "stable"}, + {Schema: "olm.package", Name: "bar", DefaultChannel: "stable"}, + }, + Channels: []declcfg.Channel{ + {Schema: "olm.channel", Name: "stable", Package: "foo", Entries: []declcfg.ChannelEntry{ + {Name: "foo.v0.1.0"}, + }}, + {Schema: "olm.channel", Name: "stable", Package: "bar", Entries: []declcfg.ChannelEntry{ + {Name: "bar.v0.1.0"}, + }}, + }, + Bundles: []declcfg.Bundle{ + { + Schema: "olm.bundle", + Name: "foo.v0.1.0", + Package: "foo", + Image: "reg/foo:latest", + Properties: []property.Property{ + property.MustBuildPackage("foo", "0.1.0"), + }, + }, + { + Schema: "olm.bundle", + Name: "bar.v0.1.0", + Package: "bar", + Image: "reg/bar:latest", + Properties: []property.Property{ + property.MustBuildGVKRequired("etcd.database.coreos.com", "v1", "EtcdBackup"), + property.MustBuildPackage("bar", "0.1.0"), + }, + }, + }, + }, + expDC: &declcfg.DeclarativeConfig{ + Packages: []declcfg.Package{ + {Schema: "olm.package", Name: "bar", DefaultChannel: "stable"}, + {Schema: "olm.package", Name: "foo", DefaultChannel: "stable"}, + }, + Channels: []declcfg.Channel{ + {Schema: "olm.channel", Name: "stable", Package: "bar", Entries: []declcfg.ChannelEntry{ + {Name: "bar.v0.1.0"}, + }}, + {Schema: "olm.channel", Name: "stable", Package: "foo", Entries: []declcfg.ChannelEntry{ + {Name: "foo.v0.1.0"}, + }}, + }, + Bundles: []declcfg.Bundle{ + { + Schema: "olm.bundle", + Name: "bar.v0.1.0", + Package: "bar", + Image: "reg/bar:latest", + Properties: []property.Property{ + property.MustBuildGVKRequired("etcd.database.coreos.com", "v1", "EtcdBackup"), + property.MustBuildPackage("bar", "0.1.0"), + }, + }, + { + Schema: "olm.bundle", + Name: "foo.v0.1.0", + Package: "foo", + Image: "reg/foo:latest", + Properties: []property.Property{ + property.MustBuildPackage("foo", "0.1.0"), + }, + }, + }, + }, + }, + { + name: "PreferLast/MergePackagesChannelsBundles", + mt: PreferLast, + dc: &declcfg.DeclarativeConfig{ + Packages: []declcfg.Package{ + {Schema: "olm.package", Name: "foo", DefaultChannel: "stable"}, + {Schema: "olm.package", Name: "foo", DefaultChannel: "alpha"}, + {Schema: "olm.package", Name: "bar", DefaultChannel: "stable"}, + }, + Channels: []declcfg.Channel{ + {Schema: "olm.channel", Name: "stable", Package: "foo", Entries: []declcfg.ChannelEntry{ + {Name: "foo.v0.1.0"}, + }}, + {Schema: "olm.channel", Name: "stable", Package: "foo", Entries: []declcfg.ChannelEntry{ + {Name: "foo.v0.5.0"}, + {Name: "foo.v0.1.0", Replaces: "foo.v0.0.5"}, + }}, + {Schema: "olm.channel", Name: "stable", Package: "bar", Entries: []declcfg.ChannelEntry{ + {Name: "bar.v0.1.0"}, + }}, + }, + Bundles: []declcfg.Bundle{ + { + Schema: "olm.bundle", + Name: "foo.v0.0.5", + Package: "foo", + Image: "reg/foo:latest", + Properties: []property.Property{ + property.MustBuildPackage("foo", "0.0.5"), + }, + }, + { + Schema: "olm.bundle", + Name: "foo.v0.1.0", + Package: "foo", + Image: "reg/foo:latest", + Properties: []property.Property{ + property.MustBuildPackage("foo", "0.1.0"), + }, + }, + { + Schema: "olm.bundle", + Name: "bar.v0.1.0", + Package: "bar", + Image: "reg/bar:latest", + Properties: []property.Property{ + property.MustBuildGVKRequired("foo.example.com", "v1", "Foo"), + property.MustBuildPackage("bar", "0.1.0"), + }, + }, + { + Schema: "olm.bundle", + Name: "bar.v0.1.0", + Package: "bar", + Image: "reg/bar:latest", + Properties: []property.Property{ + property.MustBuildGVKRequired("etcd.database.coreos.com", "v1", "EtcdBackup"), + property.MustBuildPackage("bar", "0.1.0"), + }, + }, + }, + }, + expDC: &declcfg.DeclarativeConfig{ + Packages: []declcfg.Package{ + {Schema: "olm.package", Name: "bar", DefaultChannel: "stable"}, + {Schema: "olm.package", Name: "foo", DefaultChannel: "alpha"}, + }, + Channels: []declcfg.Channel{ + {Schema: "olm.channel", Name: "stable", Package: "bar", Entries: []declcfg.ChannelEntry{ + {Name: "bar.v0.1.0"}, + }}, + {Schema: "olm.channel", Name: "stable", Package: "foo", Entries: []declcfg.ChannelEntry{ + {Name: "foo.v0.5.0"}, + {Name: "foo.v0.1.0", Replaces: "foo.v0.0.5"}, + }}, + }, + Bundles: []declcfg.Bundle{ + { + Schema: "olm.bundle", + Name: "bar.v0.1.0", + Package: "bar", + Image: "reg/bar:latest", + Properties: []property.Property{ + property.MustBuildGVKRequired("etcd.database.coreos.com", "v1", "EtcdBackup"), + property.MustBuildPackage("bar", "0.1.0"), + }, + }, + { + Schema: "olm.bundle", + Name: "foo.v0.0.5", + Package: "foo", + Image: "reg/foo:latest", + Properties: []property.Property{ + property.MustBuildPackage("foo", "0.0.5"), + }, + }, + { + Schema: "olm.bundle", + Name: "foo.v0.1.0", + Package: "foo", + Image: "reg/foo:latest", + Properties: []property.Property{ + property.MustBuildPackage("foo", "0.1.0"), + }, + }, + }, + }, + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + err := c.mt.mergeDC(c.dc) + if c.expError != "" { + require.EqualError(t, err, c.expError) + } else { + require.NoError(t, err) + require.Equal(t, c.expDC, c.dc) + } + }) + } +} diff --git a/cmd/opm/alpha/diff/cmd.go b/cmd/opm/alpha/diff/cmd.go index 752e4d343..d514753f7 100644 --- a/cmd/opm/alpha/diff/cmd.go +++ b/cmd/opm/alpha/diff/cmd.go @@ -61,6 +61,7 @@ func NewCmd() *cobra.Command { from new-refs, optionally removing those in old-refs or those omitted by an include config file. Each set of refs is passed to 'opm render ' to produce a single, normalized delcarative config. +Objects with the same unique keys are merged in ascending priority of their ref. Depending on what arguments are provided to the command, a particular "mode" is invoked to produce a diff: @@ -132,6 +133,7 @@ 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(&a.includeAdditive, "include-additive", false, "Ref objects from --include-file are returned on top of 'heads-only' or 'latest' output") + // TODO: consider exposing a.MergeType via flag, ex. --merge-type=PreferLast cmd.Flags().BoolVar(&a.debug, "debug", false, "enable debug logging") return cmd diff --git a/go.mod b/go.mod index bf9f3eaaf..6deb9c8f9 100644 --- a/go.mod +++ b/go.mod @@ -28,6 +28,7 @@ require ( github.com/grpc-ecosystem/grpc-health-probe v0.3.2 github.com/h2non/filetype v1.1.1 github.com/h2non/go-is-svg v0.0.0-20160927212452-35e8c4b0612c + github.com/imdario/mergo v0.3.12 github.com/joelanford/ignore v0.0.0-20210607151042-0d25dc18b62d github.com/mattn/go-sqlite3 v1.10.0 github.com/maxbrunsfeld/counterfeiter/v6 v6.2.2 diff --git a/vendor/modules.txt b/vendor/modules.txt index a047dfb04..b4b622913 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -310,6 +310,7 @@ github.com/h2non/filetype/types ## explicit github.com/h2non/go-is-svg # github.com/imdario/mergo v0.3.12 +## explicit github.com/imdario/mergo # github.com/inconshreveable/mousetrap v1.0.0 github.com/inconshreveable/mousetrap From 4124d2e9075544a281ba1ed8a08e6ca6879ab1e0 Mon Sep 17 00:00:00 2001 From: Jennifer Power Date: Thu, 16 Dec 2021 12:01:26 -0500 Subject: [PATCH 2/4] chore: make mergeDC function public Signed-off-by: Jennifer Power --- alpha/action/diff.go | 4 ++-- alpha/action/merge.go | 10 +++++++--- alpha/action/merge_test.go | 24 +++++++++++++++++++++++- 3 files changed, 32 insertions(+), 6 deletions(-) diff --git a/alpha/action/diff.go b/alpha/action/diff.go index 9be68d703..0e2be6b0b 100644 --- a/alpha/action/diff.go +++ b/alpha/action/diff.go @@ -53,7 +53,7 @@ func (a Diff) Run(ctx context.Context) (*declcfg.DeclarativeConfig, error) { } return nil, fmt.Errorf("error rendering old refs: %v", err) } - if err := a.MergeType.mergeDC(oldCfg); err != nil { + if err := a.MergeType.MergeDC(oldCfg); err != nil { return nil, fmt.Errorf("error merging across old refs: %v", err) } oldModel, err = declcfg.ConvertToModel(*oldCfg) @@ -70,7 +70,7 @@ func (a Diff) Run(ctx context.Context) (*declcfg.DeclarativeConfig, error) { } return nil, fmt.Errorf("error rendering new refs: %v", err) } - if err := a.MergeType.mergeDC(newCfg); err != nil { + if err := a.MergeType.MergeDC(newCfg); err != nil { return nil, fmt.Errorf("error merging across new refs: %v", err) } newModel, err := declcfg.ConvertToModel(*newCfg) diff --git a/alpha/action/merge.go b/alpha/action/merge.go index 0fd5e3344..9709bd84c 100644 --- a/alpha/action/merge.go +++ b/alpha/action/merge.go @@ -23,7 +23,7 @@ const ( TwoWay ) -func (mt MergeType) mergeDC(cfg *declcfg.DeclarativeConfig) error { +func (mt MergeType) MergeDC(cfg *declcfg.DeclarativeConfig) error { switch mt { case PreferLast: return mergeDCPreferLast(cfg) @@ -163,9 +163,13 @@ func mergeChannels(inChs []declcfg.Channel) (outChs []declcfg.Channel, err error for i, ch := range inChs { chKey := keyForDCObj(ch) chsByKey[chKey] = append(chsByKey[chKey], inChs[i]) - entriesByKey[chKey] = make(map[string][]declcfg.ChannelEntry) + entries, ok := entriesByKey[chKey] + if !ok { + entries = make(map[string][]declcfg.ChannelEntry) + entriesByKey[chKey] = entries + } for j, e := range ch.Entries { - entriesByKey[chKey][e.Name] = append(entriesByKey[chKey][e.Name], ch.Entries[j]) + entries[e.Name] = append(entries[e.Name], ch.Entries[j]) } } diff --git a/alpha/action/merge_test.go b/alpha/action/merge_test.go index 20babedc2..ac93a05ee 100644 --- a/alpha/action/merge_test.go +++ b/alpha/action/merge_test.go @@ -112,6 +112,9 @@ func TestMergeDC(t *testing.T) { {Name: "foo.v0.1.0", Replaces: "foo.v0.0.5"}, {Name: "foo.v0.1.0", Skips: []string{"foo.v0.0.4"}}, }}, + {Schema: "olm.channel", Name: "stable", Package: "foo", Entries: []declcfg.ChannelEntry{ + {Name: "foo.v0.1.1", Replaces: "foo.v1.0.0"}, + }}, {Schema: "olm.channel", Name: "stable", Package: "bar", Entries: []declcfg.ChannelEntry{ {Name: "bar.v0.1.0"}, }}, @@ -135,6 +138,15 @@ func TestMergeDC(t *testing.T) { property.MustBuildPackage("foo", "0.1.0"), }, }, + { + Schema: "olm.bundle", + Name: "foo.v0.1.1", + Package: "foo", + Image: "reg/foo:latest", + Properties: []property.Property{ + property.MustBuildPackage("foo", "0.1.1"), + }, + }, { Schema: "olm.bundle", Name: "bar.v0.1.0", @@ -168,6 +180,7 @@ func TestMergeDC(t *testing.T) { }}, {Schema: "olm.channel", Name: "stable", Package: "foo", Entries: []declcfg.ChannelEntry{ {Name: "foo.v0.1.0", Replaces: "foo.v0.0.5", Skips: []string{"foo.v0.0.4"}}, + {Name: "foo.v0.1.1", Replaces: "foo.v1.0.0"}, }}, }, Bundles: []declcfg.Bundle{ @@ -200,6 +213,15 @@ func TestMergeDC(t *testing.T) { property.MustBuildPackage("foo", "0.1.0"), }, }, + { + Schema: "olm.bundle", + Name: "foo.v0.1.1", + Package: "foo", + Image: "reg/foo:latest", + Properties: []property.Property{ + property.MustBuildPackage("foo", "0.1.1"), + }, + }, }, }, }, @@ -395,7 +417,7 @@ func TestMergeDC(t *testing.T) { for _, c := range cases { t.Run(c.name, func(t *testing.T) { - err := c.mt.mergeDC(c.dc) + err := c.mt.MergeDC(c.dc) if c.expError != "" { require.EqualError(t, err, c.expError) } else { From 1bfb42119343c3564bcdaa487e6c7c94a9b88db9 Mon Sep 17 00:00:00 2001 From: Jennifer Power Date: Thu, 20 Jan 2022 17:48:11 -0500 Subject: [PATCH 3/4] refactor(merge): move merge functionality from action to declcfg and implements as an interface This change as a Merger interface and two concrete types, PreferLast and TwoWay, that merge FBC objects. Signed-off-by: Jennifer Power --- alpha/action/diff.go | 15 ++- alpha/{action => declcfg}/merge.go | 114 ++++++++++----------- alpha/{action => declcfg}/merge_test.go | 127 ++++++++++++------------ 3 files changed, 129 insertions(+), 127 deletions(-) rename alpha/{action => declcfg}/merge.go (71%) rename alpha/{action => declcfg}/merge_test.go (84%) diff --git a/alpha/action/diff.go b/alpha/action/diff.go index 0e2be6b0b..5f6c6c7a2 100644 --- a/alpha/action/diff.go +++ b/alpha/action/diff.go @@ -29,12 +29,17 @@ type Diff struct { // IncludeAdditively catalog objects specified in IncludeConfig. IncludeAdditively bool - MergeType MergeType + Merger declcfg.Merger Logger *logrus.Entry } func (a Diff) Run(ctx context.Context) (*declcfg.DeclarativeConfig, error) { + + if a.Merger == nil { + a.Merger = &declcfg.PreferLastStrategy{} + } + if err := a.validate(); err != nil { return nil, err } @@ -53,7 +58,11 @@ func (a Diff) Run(ctx context.Context) (*declcfg.DeclarativeConfig, error) { } return nil, fmt.Errorf("error rendering old refs: %v", err) } - if err := a.MergeType.MergeDC(oldCfg); err != nil { + + if err != nil { + return nil, fmt.Errorf("error setting merge type: %v", err) + } + if err := a.Merger.MergeDC(oldCfg); err != nil { return nil, fmt.Errorf("error merging across old refs: %v", err) } oldModel, err = declcfg.ConvertToModel(*oldCfg) @@ -70,7 +79,7 @@ func (a Diff) Run(ctx context.Context) (*declcfg.DeclarativeConfig, error) { } return nil, fmt.Errorf("error rendering new refs: %v", err) } - if err := a.MergeType.MergeDC(newCfg); err != nil { + if err := a.Merger.MergeDC(newCfg); err != nil { return nil, fmt.Errorf("error merging across new refs: %v", err) } newModel, err := declcfg.ConvertToModel(*newCfg) diff --git a/alpha/action/merge.go b/alpha/declcfg/merge.go similarity index 71% rename from alpha/action/merge.go rename to alpha/declcfg/merge.go index 9709bd84c..b54ba704a 100644 --- a/alpha/action/merge.go +++ b/alpha/declcfg/merge.go @@ -1,68 +1,37 @@ -package action +package declcfg import ( "fmt" "sort" "github.com/imdario/mergo" - - "github.com/operator-framework/operator-registry/alpha/declcfg" -) - -// MergeType is the merge strategy used to combine declarative config objects -// that have the same unique key. -type MergeType int - -const ( - // PreferLast just uses the last/most recent object. - // This is the default merge type. - PreferLast MergeType = iota - // TwoWay fills in object fields by combining objects in ascending order, - // starting with the first/oldest. - // Bundle properties are NOT merged because an arbitrary property's key is unknowable. - TwoWay ) -func (mt MergeType) MergeDC(cfg *declcfg.DeclarativeConfig) error { - switch mt { - case PreferLast: - return mergeDCPreferLast(cfg) - case TwoWay: - return mergeDCTwoWay(cfg) - default: - return fmt.Errorf("unknown merge type %v", mt) - } +// Merger is an object that will complete merge actions declarative config options +type Merger interface { + MergeDC(*DeclarativeConfig) error } -func keyForDCObj(obj interface{}) string { - switch t := obj.(type) { - case declcfg.Package: - // Package name is globally unique. - return t.Name - case declcfg.Channel: - // Channel name is unqiue per package. - return t.Package + t.Name - case declcfg.Bundle: - // Bundle name is unqiue per package. - return t.Package + t.Name - default: - // This should never happen. - panic(fmt.Sprintf("bug: unrecognized type %T, expected one of Package, Channel, Bundle", t)) - } +var _ Merger = &PreferLastStrategy{} + +type PreferLastStrategy struct{} + +func (mg *PreferLastStrategy) MergeDC(dc *DeclarativeConfig) error { + return mergeDCPreferLast(dc) } // mergeDCPreferLast merges all packages, channels, and bundles with the same unique key // into single objects using the last element with that key. -func mergeDCPreferLast(cfg *declcfg.DeclarativeConfig) error { +func mergeDCPreferLast(cfg *DeclarativeConfig) error { // Merge packages. - pkgsByKey := make(map[string][]declcfg.Package, len(cfg.Packages)) + pkgsByKey := make(map[string][]Package, len(cfg.Packages)) for i, pkg := range cfg.Packages { key := keyForDCObj(pkg) pkgsByKey[key] = append(pkgsByKey[key], cfg.Packages[i]) } if len(pkgsByKey) != 0 { - outPkgs := make([]declcfg.Package, len(pkgsByKey)) + outPkgs := make([]Package, len(pkgsByKey)) i := 0 for _, pkgs := range pkgsByKey { outPkgs[i] = pkgs[len(pkgs)-1] @@ -73,13 +42,13 @@ func mergeDCPreferLast(cfg *declcfg.DeclarativeConfig) error { } // Merge channels. - chsByKey := make(map[string][]declcfg.Channel, len(cfg.Channels)) + chsByKey := make(map[string][]Channel, len(cfg.Channels)) for i, ch := range cfg.Channels { key := keyForDCObj(ch) chsByKey[key] = append(chsByKey[key], cfg.Channels[i]) } if len(chsByKey) != 0 { - outChs := make([]declcfg.Channel, len(chsByKey)) + outChs := make([]Channel, len(chsByKey)) i := 0 for _, chs := range chsByKey { outChs[i] = chs[len(chs)-1] @@ -90,13 +59,13 @@ func mergeDCPreferLast(cfg *declcfg.DeclarativeConfig) error { } // Merge bundles. - bundlesByKey := make(map[string][]declcfg.Bundle, len(cfg.Bundles)) + bundlesByKey := make(map[string][]Bundle, len(cfg.Bundles)) for i, b := range cfg.Bundles { key := keyForDCObj(b) bundlesByKey[key] = append(bundlesByKey[key], cfg.Bundles[i]) } if len(bundlesByKey) != 0 { - outBundles := make([]declcfg.Bundle, len(bundlesByKey)) + outBundles := make([]Bundle, len(bundlesByKey)) i := 0 for _, bundles := range bundlesByKey { outBundles[i] = bundles[len(bundles)-1] @@ -110,9 +79,17 @@ func mergeDCPreferLast(cfg *declcfg.DeclarativeConfig) error { return nil } +var _ Merger = &TwoWayStrategy{} + +type TwoWayStrategy struct{} + +func (mg *TwoWayStrategy) MergeDC(dc *DeclarativeConfig) error { + return mergeDCTwoWay(dc) +} + // mergeDCTwoWay merges all packages, channels, and bundles with the same unique key // into single objects with ascending priority. -func mergeDCTwoWay(cfg *declcfg.DeclarativeConfig) error { +func mergeDCTwoWay(cfg *DeclarativeConfig) error { var err error if cfg.Packages, err = mergePackages(cfg.Packages); err != nil { return err @@ -129,8 +106,8 @@ func mergeDCTwoWay(cfg *declcfg.DeclarativeConfig) error { // mergePackages merges all packages with the same name into one package object. // Value preference is ascending: values of packages later in input are preferred. -func mergePackages(inPkgs []declcfg.Package) (outPkgs []declcfg.Package, err error) { - pkgsByName := make(map[string][]declcfg.Package, len(inPkgs)) +func mergePackages(inPkgs []Package) (outPkgs []Package, err error) { + pkgsByName := make(map[string][]Package, len(inPkgs)) for i, pkg := range inPkgs { key := keyForDCObj(pkg) pkgsByName[key] = append(pkgsByName[key], inPkgs[i]) @@ -157,15 +134,15 @@ func mergePackages(inPkgs []declcfg.Package) (outPkgs []declcfg.Package, err err // mergeChannels merges all channels with the same name and package into one channel object. // Value preference is ascending: values of channels later in input are preferred. -func mergeChannels(inChs []declcfg.Channel) (outChs []declcfg.Channel, err error) { - chsByKey := make(map[string][]declcfg.Channel, len(inChs)) - entriesByKey := make(map[string]map[string][]declcfg.ChannelEntry, len(inChs)) +func mergeChannels(inChs []Channel) (outChs []Channel, err error) { + chsByKey := make(map[string][]Channel, len(inChs)) + entriesByKey := make(map[string]map[string][]ChannelEntry, len(inChs)) for i, ch := range inChs { chKey := keyForDCObj(ch) chsByKey[chKey] = append(chsByKey[chKey], inChs[i]) entries, ok := entriesByKey[chKey] if !ok { - entries = make(map[string][]declcfg.ChannelEntry) + entries = make(map[string][]ChannelEntry) entriesByKey[chKey] = entries } for j, e := range ch.Entries { @@ -213,8 +190,8 @@ func mergeChannels(inChs []declcfg.Channel) (outChs []declcfg.Channel, err error // mergeBundles merges all bundles with the same name and package into one bundle object. // Value preference is ascending: values of bundles later in input are preferred. -func mergeBundles(inBundles []declcfg.Bundle) (outBundles []declcfg.Bundle, err error) { - bundlesByKey := make(map[string][]declcfg.Bundle, len(inBundles)) +func mergeBundles(inBundles []Bundle) (outBundles []Bundle, err error) { + bundlesByKey := make(map[string][]Bundle, len(inBundles)) for i, bundle := range inBundles { key := keyForDCObj(bundle) bundlesByKey[key] = append(bundlesByKey[key], inBundles[i]) @@ -239,13 +216,13 @@ func mergeBundles(inBundles []declcfg.Bundle) (outBundles []declcfg.Bundle, err return outBundles, nil } -func sortPackages(pkgs []declcfg.Package) { +func sortPackages(pkgs []Package) { sort.Slice(pkgs, func(i, j int) bool { return pkgs[i].Name < pkgs[j].Name }) } -func sortChannels(chs []declcfg.Channel) { +func sortChannels(chs []Channel) { sort.Slice(chs, func(i, j int) bool { if chs[i].Package == chs[j].Package { return chs[i].Name < chs[j].Name @@ -254,7 +231,7 @@ func sortChannels(chs []declcfg.Channel) { }) } -func sortBundles(bundles []declcfg.Bundle) { +func sortBundles(bundles []Bundle) { sort.Slice(bundles, func(i, j int) bool { if bundles[i].Package == bundles[j].Package { return bundles[i].Name < bundles[j].Name @@ -262,3 +239,20 @@ func sortBundles(bundles []declcfg.Bundle) { return bundles[i].Package < bundles[j].Package }) } + +func keyForDCObj(obj interface{}) string { + switch t := obj.(type) { + case Package: + // Package name is globally unique. + return t.Name + case Channel: + // Channel name is unqiue per package. + return t.Package + t.Name + case Bundle: + // Bundle name is unqiue per package. + return t.Package + t.Name + default: + // This should never happen. + panic(fmt.Sprintf("bug: unrecognized type %T, expected one of Package, Channel, Bundle", t)) + } +} diff --git a/alpha/action/merge_test.go b/alpha/declcfg/merge_test.go similarity index 84% rename from alpha/action/merge_test.go rename to alpha/declcfg/merge_test.go index ac93a05ee..ed83ddfa1 100644 --- a/alpha/action/merge_test.go +++ b/alpha/declcfg/merge_test.go @@ -1,9 +1,8 @@ -package action +package declcfg import ( "testing" - "github.com/operator-framework/operator-registry/alpha/declcfg" "github.com/operator-framework/operator-registry/alpha/property" "github.com/stretchr/testify/require" ) @@ -11,35 +10,35 @@ import ( func TestMergeDC(t *testing.T) { type spec struct { name string - mt MergeType - dc, expDC *declcfg.DeclarativeConfig + mt Merger + dc, expDC *DeclarativeConfig expError string } cases := []spec{ { name: "TwoWay/Empty", - mt: TwoWay, - dc: &declcfg.DeclarativeConfig{}, - expDC: &declcfg.DeclarativeConfig{}, + mt: &TwoWayStrategy{}, + dc: &DeclarativeConfig{}, + expDC: &DeclarativeConfig{}, }, { name: "TwoWay/NoMergeNeeded", - mt: TwoWay, - dc: &declcfg.DeclarativeConfig{ - Packages: []declcfg.Package{ + mt: &TwoWayStrategy{}, + dc: &DeclarativeConfig{ + Packages: []Package{ {Schema: "olm.package", Name: "foo", DefaultChannel: "stable"}, {Schema: "olm.package", Name: "bar", DefaultChannel: "stable"}, }, - Channels: []declcfg.Channel{ - {Schema: "olm.channel", Name: "stable", Package: "foo", Entries: []declcfg.ChannelEntry{ + Channels: []Channel{ + {Schema: "olm.channel", Name: "stable", Package: "foo", Entries: []ChannelEntry{ {Name: "foo.v0.1.0"}, }}, - {Schema: "olm.channel", Name: "stable", Package: "bar", Entries: []declcfg.ChannelEntry{ + {Schema: "olm.channel", Name: "stable", Package: "bar", Entries: []ChannelEntry{ {Name: "bar.v0.1.0"}, }}, }, - Bundles: []declcfg.Bundle{ + Bundles: []Bundle{ { Schema: "olm.bundle", Name: "foo.v0.1.0", @@ -61,20 +60,20 @@ func TestMergeDC(t *testing.T) { }, }, }, - expDC: &declcfg.DeclarativeConfig{ - Packages: []declcfg.Package{ + expDC: &DeclarativeConfig{ + Packages: []Package{ {Schema: "olm.package", Name: "bar", DefaultChannel: "stable"}, {Schema: "olm.package", Name: "foo", DefaultChannel: "stable"}, }, - Channels: []declcfg.Channel{ - {Schema: "olm.channel", Name: "stable", Package: "bar", Entries: []declcfg.ChannelEntry{ + Channels: []Channel{ + {Schema: "olm.channel", Name: "stable", Package: "bar", Entries: []ChannelEntry{ {Name: "bar.v0.1.0"}, }}, - {Schema: "olm.channel", Name: "stable", Package: "foo", Entries: []declcfg.ChannelEntry{ + {Schema: "olm.channel", Name: "stable", Package: "foo", Entries: []ChannelEntry{ {Name: "foo.v0.1.0"}, }}, }, - Bundles: []declcfg.Bundle{ + Bundles: []Bundle{ { Schema: "olm.bundle", Name: "bar.v0.1.0", @@ -99,27 +98,27 @@ func TestMergeDC(t *testing.T) { }, { name: "TwoWay/MergePackagesChannelsBundles", - mt: TwoWay, - dc: &declcfg.DeclarativeConfig{ - Packages: []declcfg.Package{ + mt: &TwoWayStrategy{}, + dc: &DeclarativeConfig{ + Packages: []Package{ {Schema: "olm.package", Name: "foo", DefaultChannel: "stable"}, {Schema: "olm.package", Name: "foo", DefaultChannel: "alpha"}, {Schema: "olm.package", Name: "bar", DefaultChannel: "stable"}, }, - Channels: []declcfg.Channel{ - {Schema: "olm.channel", Name: "stable", Package: "foo", Entries: []declcfg.ChannelEntry{ + Channels: []Channel{ + {Schema: "olm.channel", Name: "stable", Package: "foo", Entries: []ChannelEntry{ {Name: "foo.v0.1.0"}, {Name: "foo.v0.1.0", Replaces: "foo.v0.0.5"}, {Name: "foo.v0.1.0", Skips: []string{"foo.v0.0.4"}}, }}, - {Schema: "olm.channel", Name: "stable", Package: "foo", Entries: []declcfg.ChannelEntry{ + {Schema: "olm.channel", Name: "stable", Package: "foo", Entries: []ChannelEntry{ {Name: "foo.v0.1.1", Replaces: "foo.v1.0.0"}, }}, - {Schema: "olm.channel", Name: "stable", Package: "bar", Entries: []declcfg.ChannelEntry{ + {Schema: "olm.channel", Name: "stable", Package: "bar", Entries: []ChannelEntry{ {Name: "bar.v0.1.0"}, }}, }, - Bundles: []declcfg.Bundle{ + Bundles: []Bundle{ { Schema: "olm.bundle", Name: "foo.v0.0.5", @@ -169,21 +168,21 @@ func TestMergeDC(t *testing.T) { }, }, }, - expDC: &declcfg.DeclarativeConfig{ - Packages: []declcfg.Package{ + expDC: &DeclarativeConfig{ + Packages: []Package{ {Schema: "olm.package", Name: "bar", DefaultChannel: "stable"}, {Schema: "olm.package", Name: "foo", DefaultChannel: "alpha"}, }, - Channels: []declcfg.Channel{ - {Schema: "olm.channel", Name: "stable", Package: "bar", Entries: []declcfg.ChannelEntry{ + Channels: []Channel{ + {Schema: "olm.channel", Name: "stable", Package: "bar", Entries: []ChannelEntry{ {Name: "bar.v0.1.0"}, }}, - {Schema: "olm.channel", Name: "stable", Package: "foo", Entries: []declcfg.ChannelEntry{ + {Schema: "olm.channel", Name: "stable", Package: "foo", Entries: []ChannelEntry{ {Name: "foo.v0.1.0", Replaces: "foo.v0.0.5", Skips: []string{"foo.v0.0.4"}}, {Name: "foo.v0.1.1", Replaces: "foo.v1.0.0"}, }}, }, - Bundles: []declcfg.Bundle{ + Bundles: []Bundle{ { Schema: "olm.bundle", Name: "bar.v0.1.0", @@ -227,27 +226,27 @@ func TestMergeDC(t *testing.T) { }, { name: "PreferLast/Empty", - mt: PreferLast, - dc: &declcfg.DeclarativeConfig{}, - expDC: &declcfg.DeclarativeConfig{}, + mt: &PreferLastStrategy{}, + dc: &DeclarativeConfig{}, + expDC: &DeclarativeConfig{}, }, { name: "PreferLast/NoMergeNeeded", - mt: PreferLast, - dc: &declcfg.DeclarativeConfig{ - Packages: []declcfg.Package{ + mt: &PreferLastStrategy{}, + dc: &DeclarativeConfig{ + Packages: []Package{ {Schema: "olm.package", Name: "foo", DefaultChannel: "stable"}, {Schema: "olm.package", Name: "bar", DefaultChannel: "stable"}, }, - Channels: []declcfg.Channel{ - {Schema: "olm.channel", Name: "stable", Package: "foo", Entries: []declcfg.ChannelEntry{ + Channels: []Channel{ + {Schema: "olm.channel", Name: "stable", Package: "foo", Entries: []ChannelEntry{ {Name: "foo.v0.1.0"}, }}, - {Schema: "olm.channel", Name: "stable", Package: "bar", Entries: []declcfg.ChannelEntry{ + {Schema: "olm.channel", Name: "stable", Package: "bar", Entries: []ChannelEntry{ {Name: "bar.v0.1.0"}, }}, }, - Bundles: []declcfg.Bundle{ + Bundles: []Bundle{ { Schema: "olm.bundle", Name: "foo.v0.1.0", @@ -269,20 +268,20 @@ func TestMergeDC(t *testing.T) { }, }, }, - expDC: &declcfg.DeclarativeConfig{ - Packages: []declcfg.Package{ + expDC: &DeclarativeConfig{ + Packages: []Package{ {Schema: "olm.package", Name: "bar", DefaultChannel: "stable"}, {Schema: "olm.package", Name: "foo", DefaultChannel: "stable"}, }, - Channels: []declcfg.Channel{ - {Schema: "olm.channel", Name: "stable", Package: "bar", Entries: []declcfg.ChannelEntry{ + Channels: []Channel{ + {Schema: "olm.channel", Name: "stable", Package: "bar", Entries: []ChannelEntry{ {Name: "bar.v0.1.0"}, }}, - {Schema: "olm.channel", Name: "stable", Package: "foo", Entries: []declcfg.ChannelEntry{ + {Schema: "olm.channel", Name: "stable", Package: "foo", Entries: []ChannelEntry{ {Name: "foo.v0.1.0"}, }}, }, - Bundles: []declcfg.Bundle{ + Bundles: []Bundle{ { Schema: "olm.bundle", Name: "bar.v0.1.0", @@ -307,26 +306,26 @@ func TestMergeDC(t *testing.T) { }, { name: "PreferLast/MergePackagesChannelsBundles", - mt: PreferLast, - dc: &declcfg.DeclarativeConfig{ - Packages: []declcfg.Package{ + mt: &PreferLastStrategy{}, + dc: &DeclarativeConfig{ + Packages: []Package{ {Schema: "olm.package", Name: "foo", DefaultChannel: "stable"}, {Schema: "olm.package", Name: "foo", DefaultChannel: "alpha"}, {Schema: "olm.package", Name: "bar", DefaultChannel: "stable"}, }, - Channels: []declcfg.Channel{ - {Schema: "olm.channel", Name: "stable", Package: "foo", Entries: []declcfg.ChannelEntry{ + Channels: []Channel{ + {Schema: "olm.channel", Name: "stable", Package: "foo", Entries: []ChannelEntry{ {Name: "foo.v0.1.0"}, }}, - {Schema: "olm.channel", Name: "stable", Package: "foo", Entries: []declcfg.ChannelEntry{ + {Schema: "olm.channel", Name: "stable", Package: "foo", Entries: []ChannelEntry{ {Name: "foo.v0.5.0"}, {Name: "foo.v0.1.0", Replaces: "foo.v0.0.5"}, }}, - {Schema: "olm.channel", Name: "stable", Package: "bar", Entries: []declcfg.ChannelEntry{ + {Schema: "olm.channel", Name: "stable", Package: "bar", Entries: []ChannelEntry{ {Name: "bar.v0.1.0"}, }}, }, - Bundles: []declcfg.Bundle{ + Bundles: []Bundle{ { Schema: "olm.bundle", Name: "foo.v0.0.5", @@ -367,21 +366,21 @@ func TestMergeDC(t *testing.T) { }, }, }, - expDC: &declcfg.DeclarativeConfig{ - Packages: []declcfg.Package{ + expDC: &DeclarativeConfig{ + Packages: []Package{ {Schema: "olm.package", Name: "bar", DefaultChannel: "stable"}, {Schema: "olm.package", Name: "foo", DefaultChannel: "alpha"}, }, - Channels: []declcfg.Channel{ - {Schema: "olm.channel", Name: "stable", Package: "bar", Entries: []declcfg.ChannelEntry{ + Channels: []Channel{ + {Schema: "olm.channel", Name: "stable", Package: "bar", Entries: []ChannelEntry{ {Name: "bar.v0.1.0"}, }}, - {Schema: "olm.channel", Name: "stable", Package: "foo", Entries: []declcfg.ChannelEntry{ + {Schema: "olm.channel", Name: "stable", Package: "foo", Entries: []ChannelEntry{ {Name: "foo.v0.5.0"}, {Name: "foo.v0.1.0", Replaces: "foo.v0.0.5"}, }}, }, - Bundles: []declcfg.Bundle{ + Bundles: []Bundle{ { Schema: "olm.bundle", Name: "bar.v0.1.0", From 135279794a213e53588586224802ac327a07f7e7 Mon Sep 17 00:00:00 2001 From: Jennifer Power Date: Fri, 21 Jan 2022 12:55:11 -0500 Subject: [PATCH 4/4] chore: change MergeDC function to Merge for clarity Signed-off-by: Jennifer Power --- alpha/action/diff.go | 8 ++------ alpha/declcfg/merge.go | 6 +++--- alpha/declcfg/merge_test.go | 2 +- 3 files changed, 6 insertions(+), 10 deletions(-) diff --git a/alpha/action/diff.go b/alpha/action/diff.go index 5f6c6c7a2..f074ac0d8 100644 --- a/alpha/action/diff.go +++ b/alpha/action/diff.go @@ -58,11 +58,7 @@ func (a Diff) Run(ctx context.Context) (*declcfg.DeclarativeConfig, error) { } return nil, fmt.Errorf("error rendering old refs: %v", err) } - - if err != nil { - return nil, fmt.Errorf("error setting merge type: %v", err) - } - if err := a.Merger.MergeDC(oldCfg); err != nil { + if err := a.Merger.Merge(oldCfg); err != nil { return nil, fmt.Errorf("error merging across old refs: %v", err) } oldModel, err = declcfg.ConvertToModel(*oldCfg) @@ -79,7 +75,7 @@ func (a Diff) Run(ctx context.Context) (*declcfg.DeclarativeConfig, error) { } return nil, fmt.Errorf("error rendering new refs: %v", err) } - if err := a.Merger.MergeDC(newCfg); err != nil { + if err := a.Merger.Merge(newCfg); err != nil { return nil, fmt.Errorf("error merging across new refs: %v", err) } newModel, err := declcfg.ConvertToModel(*newCfg) diff --git a/alpha/declcfg/merge.go b/alpha/declcfg/merge.go index b54ba704a..99507095e 100644 --- a/alpha/declcfg/merge.go +++ b/alpha/declcfg/merge.go @@ -9,14 +9,14 @@ import ( // Merger is an object that will complete merge actions declarative config options type Merger interface { - MergeDC(*DeclarativeConfig) error + Merge(*DeclarativeConfig) error } var _ Merger = &PreferLastStrategy{} type PreferLastStrategy struct{} -func (mg *PreferLastStrategy) MergeDC(dc *DeclarativeConfig) error { +func (mg *PreferLastStrategy) Merge(dc *DeclarativeConfig) error { return mergeDCPreferLast(dc) } @@ -83,7 +83,7 @@ var _ Merger = &TwoWayStrategy{} type TwoWayStrategy struct{} -func (mg *TwoWayStrategy) MergeDC(dc *DeclarativeConfig) error { +func (mg *TwoWayStrategy) Merge(dc *DeclarativeConfig) error { return mergeDCTwoWay(dc) } diff --git a/alpha/declcfg/merge_test.go b/alpha/declcfg/merge_test.go index ed83ddfa1..1e6792bd5 100644 --- a/alpha/declcfg/merge_test.go +++ b/alpha/declcfg/merge_test.go @@ -416,7 +416,7 @@ func TestMergeDC(t *testing.T) { for _, c := range cases { t.Run(c.name, func(t *testing.T) { - err := c.mt.MergeDC(c.dc) + err := c.mt.Merge(c.dc) if c.expError != "" { require.EqualError(t, err, c.expError) } else {