diff --git a/alpha/action/diff.go b/alpha/action/diff.go index 7912cf8a7..9cae4dedc 100644 --- a/alpha/action/diff.go +++ b/alpha/action/diff.go @@ -32,8 +32,8 @@ type Diff struct { Logger *logrus.Entry } -func (a Diff) Run(ctx context.Context) (*declcfg.DeclarativeConfig, error) { - if err := a.validate(); err != nil { +func (diff Diff) Run(ctx context.Context) (*declcfg.DeclarativeConfig, error) { + if err := diff.validate(); err != nil { return nil, err } @@ -42,8 +42,8 @@ func (a Diff) Run(ctx context.Context) (*declcfg.DeclarativeConfig, error) { // Heads-only mode does not require an old ref, so there may be nothing to render. var oldModel model.Model - if len(a.OldRefs) != 0 { - oldRender := Render{Refs: a.OldRefs, Registry: a.Registry, AllowedRefMask: mask} + if len(diff.OldRefs) != 0 { + oldRender := Render{Refs: diff.OldRefs, Registry: diff.Registry, AllowedRefMask: mask} oldCfg, err := oldRender.Run(ctx) if err != nil { if errors.Is(err, ErrNotAllowed) { @@ -57,7 +57,7 @@ func (a Diff) Run(ctx context.Context) (*declcfg.DeclarativeConfig, error) { } } - newRender := Render{Refs: a.NewRefs, Registry: a.Registry, AllowedRefMask: mask} + newRender := Render{Refs: diff.NewRefs, Registry: diff.Registry, AllowedRefMask: mask} newCfg, err := newRender.Run(ctx) if err != nil { if errors.Is(err, ErrNotAllowed) { @@ -71,10 +71,10 @@ func (a Diff) Run(ctx context.Context) (*declcfg.DeclarativeConfig, error) { } g := &declcfg.DiffGenerator{ - Logger: a.Logger, - SkipDependencies: a.SkipDependencies, - Includer: convertIncludeConfigToIncluder(a.IncludeConfig), - IncludeAdditively: a.IncludeAdditively, + Logger: diff.Logger, + SkipDependencies: diff.SkipDependencies, + Includer: convertIncludeConfigToIncluder(diff.IncludeConfig), + IncludeAdditively: diff.IncludeAdditively, } diffModel, err := g.Run(oldModel, newModel) if err != nil { diff --git a/alpha/declcfg/diff.go b/alpha/declcfg/diff.go index 351eb95e5..86bb83332 100644 --- a/alpha/declcfg/diff.go +++ b/alpha/declcfg/diff.go @@ -242,8 +242,10 @@ func bundlesEqual(b1, b2 *model.Bundle) (bool, error) { func addAllDependencies(newModel, oldModel, outputModel model.Model) error { // Get every oldModel's bundle's dependencies, and their dependencies, etc. by BFS. providingBundlesByPackage := map[string][]*model.Bundle{} - for curr := getBundles(outputModel); len(curr) != 0; { - reqGVKs, reqPkgs, err := findDependencies(curr) + var visitedBundles []*model.Bundle + for currentList := getBundles(outputModel); len(currentList) != 0; { + visitedBundles = append(visitedBundles, currentList...) + reqGVKs, reqPkgs, err := findDependencies(currentList) if err != nil { return err } @@ -251,15 +253,16 @@ func addAllDependencies(newModel, oldModel, outputModel model.Model) error { if len(reqGVKs) == 0 && len(reqPkgs) == 0 { break } - curr = nil + currentList = nil // Get bundles that provide dependencies from newModel, which should have // the latest bundles of each dependency package. for _, pkg := range newModel { providingBundles := getBundlesThatProvide(pkg, reqGVKs, reqPkgs) - curr = append(curr, providingBundles...) + unvisitedProvidingBundles := difference(visitedBundles, providingBundles) + currentList = append(currentList, unvisitedProvidingBundles...) oldPkg, oldHasPkg := oldModel[pkg.Name] - for _, b := range providingBundles { + for _, b := range unvisitedProvidingBundles { // If the bundle is not in oldModel, add it to the set. // outputModel is checked below. add := true @@ -341,6 +344,20 @@ func addAllDependencies(newModel, oldModel, outputModel model.Model) error { return nil } +func difference(a, b []*model.Bundle) []*model.Bundle { + aMap := make(map[*model.Bundle]struct{}) + for _, bd := range a { + aMap[bd] = struct{}{} + } + uniqueBundles := make([]*model.Bundle, 0) + for _, bd := range b { + if _, present := aMap[bd]; !present { + uniqueBundles = append(uniqueBundles, bd) + } + } + return uniqueBundles +} + // getBundles collects all bundles specified by m. Since each bundle // references its package, their uniqueness property holds in a flat list. func getBundles(m model.Model) (bundles []*model.Bundle) { diff --git a/alpha/declcfg/diff_test.go b/alpha/declcfg/diff_test.go index d36e2aadd..f9128e81e 100644 --- a/alpha/declcfg/diff_test.go +++ b/alpha/declcfg/diff_test.go @@ -1519,6 +1519,86 @@ func TestDiffHeadsOnly(t *testing.T) { }, }, }, + { + name: "HasDiff/CyclicDependencyGraph", + newCfg: DeclarativeConfig{ + Packages: []Package{ + {Schema: schemaPackage, Name: "foo", DefaultChannel: "stable"}, + {Schema: schemaPackage, Name: "bar", DefaultChannel: "stable"}, + }, + Channels: []Channel{ + {Schema: schemaChannel, Name: "stable", Package: "bar", Entries: []ChannelEntry{ + {Name: "bar.v4.9.3"}, + }}, + {Schema: schemaChannel, Name: "stable", Package: "foo", Entries: []ChannelEntry{ + {Name: "foo.v4.9.3"}, + }}, + }, + Bundles: []Bundle{ + { + Schema: schemaBundle, + Name: "foo.v4.9.3", + Package: "foo", + Image: "reg/foo:latest", + Properties: []property.Property{ + property.MustBuildPackage("foo", "0.1.0"), + property.MustBuildGVK("foo", "v1alpha1", "Foo"), + property.MustBuildGVKRequired("bar", "v1alpha1", "Bar"), + }, + }, + { + Schema: schemaBundle, + Name: "bar.v4.9.3", + Package: "bar", + Image: "reg/bar:latest", + Properties: []property.Property{ + property.MustBuildPackage("bar", "4.9.3"), + property.MustBuildGVK("bar", "v1alpha1", "Bar"), + property.MustBuildGVKRequired("foo", "v1alpha1", "Foo"), + }, + }, + }, + }, + g: &DiffGenerator{}, + expCfg: DeclarativeConfig{ + Packages: []Package{ + {Schema: schemaPackage, Name: "bar", DefaultChannel: "stable"}, + {Schema: schemaPackage, Name: "foo", DefaultChannel: "stable"}, + }, + Channels: []Channel{ + {Schema: schemaChannel, Name: "stable", Package: "bar", Entries: []ChannelEntry{ + {Name: "bar.v4.9.3"}, + }}, + {Schema: schemaChannel, Name: "stable", Package: "foo", Entries: []ChannelEntry{ + {Name: "foo.v4.9.3"}, + }}, + }, + Bundles: []Bundle{ + { + Schema: schemaBundle, + Name: "bar.v4.9.3", + Package: "bar", + Image: "reg/bar:latest", + Properties: []property.Property{ + property.MustBuildGVK("bar", "v1alpha1", "Bar"), + property.MustBuildGVKRequired("foo", "v1alpha1", "Foo"), + property.MustBuildPackage("bar", "4.9.3"), + }, + }, + { + Schema: schemaBundle, + Name: "foo.v4.9.3", + Package: "foo", + Image: "reg/foo:latest", + Properties: []property.Property{ + property.MustBuildGVK("foo", "v1alpha1", "Foo"), + property.MustBuildGVKRequired("bar", "v1alpha1", "Bar"), + property.MustBuildPackage("foo", "0.1.0"), + }, + }, + }, + }, + }, { // Testing SkipDependencies only really makes sense in heads-only mode, // since new dependencies are always added. diff --git a/cmd/opm/alpha/diff/cmd.go b/cmd/opm/alpha/diff/cmd.go index 54ed5aa54..02cd52b28 100644 --- a/cmd/opm/alpha/diff/cmd.go +++ b/cmd/opm/alpha/diff/cmd.go @@ -24,7 +24,7 @@ const ( timeout = time.Minute * 1 ) -type diff struct { +type input struct { oldRefs []string newRefs []string skipDeps bool @@ -51,7 +51,7 @@ var includeFileExample = fmt.Sprintf(`packages: %[1]s - 0.2.0-alpha.0`, templates.Indentation) func NewCmd() *cobra.Command { - a := diff{ + in := input{ logger: logrus.NewEntry(logrus.New()), } cmd := &cobra.Command{ @@ -115,44 +115,44 @@ docker push registry.org/my-catalog:diff-latest `), includeFileExample), Args: cobra.RangeArgs(1, 2), PreRunE: func(cmd *cobra.Command, args []string) error { - if a.debug { - a.logger.Logger.SetLevel(logrus.DebugLevel) + if in.debug { + in.logger.Logger.SetLevel(logrus.DebugLevel) } - a.logger.Logger.SetOutput(os.Stderr) + in.logger.Logger.SetOutput(os.Stderr) return nil }, - RunE: a.addFunc, + RunE: in.diffFunc, } - cmd.Flags().BoolVar(&a.skipDeps, "skip-deps", false, "do not include bundle dependencies in the output catalog") + cmd.Flags().BoolVar(&in.skipDeps, "skip-deps", false, "do not include bundle dependencies in the output catalog") - cmd.Flags().StringVarP(&a.output, "output", "o", "yaml", "Output format (json|yaml)") - cmd.Flags().StringVar(&a.caFile, "ca-file", "", "the root Certificates to use with this command") - cmd.Flags().StringVarP(&a.includeFile, "include-file", "i", "", + cmd.Flags().StringVarP(&in.output, "output", "o", "yaml", "Output format (json|yaml)") + cmd.Flags().StringVar(&in.caFile, "ca-file", "", "the root Certificates to use with this command") + cmd.Flags().StringVarP(&in.includeFile, "include-file", "i", "", "YAML defining packages, channels, and/or bundles/versions to extract from the new refs. "+ "Upgrade graphs from individual bundles/versions to their channel's head are also included") - cmd.Flags().BoolVar(&a.includeAdditive, "include-additive", false, + 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(&a.debug, "debug", false, "enable debug logging") + cmd.Flags().BoolVar(&in.debug, "debug", false, "enable debug logging") return cmd } -func (a *diff) addFunc(cmd *cobra.Command, args []string) error { - a.parseArgs(args) +func (in *input) diffFunc(cmd *cobra.Command, args []string) error { + in.parseArgs(args) - if cmd.Flags().Changed("include-additive") && a.includeFile == "" { - a.logger.Fatal("must set --include-file if --include-additive is set") + if cmd.Flags().Changed("include-additive") && in.includeFile == "" { + in.logger.Fatal("must set --include-file if --include-additive is set") } var write func(declcfg.DeclarativeConfig, io.Writer) error - switch a.output { + switch in.output { case "yaml": write = declcfg.WriteYAML case "json": write = declcfg.WriteJSON default: - return fmt.Errorf("invalid --output value: %q", a.output) + return fmt.Errorf("invalid --output value: %q", in.output) } skipTLSVerify, useHTTP, err := util.GetTLSOptions(cmd) @@ -160,46 +160,46 @@ func (a *diff) addFunc(cmd *cobra.Command, args []string) error { return err } - rootCAs, err := certs.RootCAs(a.caFile) + rootCAs, err := certs.RootCAs(in.caFile) if err != nil { - a.logger.Fatalf("error getting root CAs: %v", err) + in.logger.Fatalf("error getting root CAs: %v", err) } reg, err := containerd.NewRegistry( containerd.SkipTLSVerify(skipTLSVerify), - containerd.WithLog(a.logger), + containerd.WithLog(in.logger), containerd.WithRootCAs(rootCAs), containerd.WithPlainHTTP(useHTTP), ) if err != nil { - a.logger.Fatalf("error creating containerd registry: %v", err) + in.logger.Fatalf("error creating containerd registry: %v", err) } defer func() { if err := reg.Destroy(); err != nil { - a.logger.Errorf("error destroying local cache: %v", err) + in.logger.Errorf("error destroying local cache: %v", err) } }() diff := action.Diff{ Registry: reg, - OldRefs: a.oldRefs, - NewRefs: a.newRefs, - SkipDependencies: a.skipDeps, - IncludeAdditively: a.includeAdditive, - Logger: a.logger, + OldRefs: in.oldRefs, + NewRefs: in.newRefs, + SkipDependencies: in.skipDeps, + IncludeAdditively: in.includeAdditive, + Logger: in.logger, } - if a.includeFile != "" { - f, err := os.Open(a.includeFile) + if in.includeFile != "" { + f, err := os.Open(in.includeFile) if err != nil { - a.logger.Fatalf("error opening include file: %v", err) + in.logger.Fatalf("error opening include file: %v", err) } defer func() { if cerr := f.Close(); cerr != nil { - a.logger.Error(cerr) + in.logger.Error(cerr) } }() if diff.IncludeConfig, err = action.LoadDiffIncludeConfig(f); err != nil { - a.logger.Fatalf("error loading include file: %v", err) + in.logger.Fatalf("error loading include file: %v", err) } } @@ -208,17 +208,17 @@ func (a *diff) addFunc(cmd *cobra.Command, args []string) error { cfg, err := diff.Run(ctx) if err != nil { - a.logger.Fatalf("error generating diff: %v", err) + in.logger.Fatalf("error generating diff: %v", err) } if err := write(*cfg, os.Stdout); err != nil { - a.logger.Fatalf("error writing diff: %v", err) + in.logger.Fatalf("error writing diff: %v", err) } return nil } -func (a *diff) parseArgs(args []string) { +func (in *input) parseArgs(args []string) { var old, new string switch len(args) { case 1: @@ -229,7 +229,7 @@ func (a *diff) parseArgs(args []string) { logrus.Panic("should never be here, CLI must enforce arg size") } if old != "" { - a.oldRefs = strings.Split(old, ",") + in.oldRefs = strings.Split(old, ",") } - a.newRefs = strings.Split(new, ",") + in.newRefs = strings.Split(new, ",") }