[feature] Understanding packages from bundle directory #241

Bowenislandsong · 2020-03-29T17:26:15Z

This commit adds a feature to interpret bundle name and channels
based on a given bundle directory and update bundle's annotation
YAML. This feature is triggered when opm alpha generate/build is
not supplying packageName, channels, and defaultChannels information.
This Bundle directory needs to be inside a Package directory
that contains all bundles.

The previous reverted commit 13373cb
accidentally changed opm command.
This commit removes the duplicated package flag.
This commit corrects bundle dir from pakcgae directory.
This commit adds an error message for getting empty channels for a bundle.
This commit skipes error for trying to parse anyfile as Json or Yaml format.

Description of the change:

Motivation for the change:

Reviewer Checklist

Implementation matches the proposed design, or proposal is updated to match implementation
Sufficient unit test coverage
Sufficient end-to-end test coverage
Docs updated or added to /docs
Commit messages sensible and descriptive

Bowenislandsong · 2020-03-29T18:58:51Z

/retest

Bowenislandsong · 2020-03-29T20:18:52Z

/retest

Bowenislandsong · 2020-03-30T01:42:14Z

/cc @kevinrizza @ecordell @dinhxuanvu @awgreene

ecordell

/hold

The package inference looks good, but I have concerns about using the graph loader here - please see comments inline.

ecordell · 2020-03-30T12:52:11Z

pkg/registry/csv.go

+
+		return &csv, nil
+	}
+	return nil, fmt.Errorf("csv typed YAML file not found in %s", bundleDir)


nit: no ClusterServiceVersion object found in %s (since this parses yaml/json, don't indicate to users that it must be yaml)

ecordell · 2020-03-30T13:01:12Z

pkg/lib/bundle/interpreter.go

+		return nil, fmt.Errorf("error loading CSV from bundle directory, %v", err)
+	}
+
+	pkgDir, err := registry.NewPackageGraphLoaderFromDir(path.Join(bundleDir, ".."))


I'm not sure this is what we want to do here.

This reads in the directory of files and generates a graph, and tells you if there are problems.

But the generate command is used for a single bundle. That single bundle may be applied to any target index, and has nothing to do with the bundles that happen to be in in the filesystem next to it.

I think this would be fine without the graph loader in this PR at all. Thoughts?

But I thought the whole purpose of this though is to automagically decide what channels a given bundle is. How can we do that without attempting to build the graph for a given package?

Thanks for the explanation - commented before ☕️ !

I think there's a slight impedance mismatch here - this option is now only really useful if you're attempting convert an entire directory of the old format, right?

Should this really be part of the normal command for generating/buliding single bundles when it's so tied to the set?

I think the trigger for it should be "if you didn't pass in these parameters explicitly, let's go see if we can find them for you" rather than it being an explicit opt in/opt out feature. IMO that's a better experience

I second that this should be an experience enhancement type of feature based on the fact that you have the full-blown format with bundles nested in a package along with other bundles. Without it, it does not have any information we would be looking for. The default channel and channels are only obtainable in the *.package.yaml file. To figure out which channels a bundle belongs to require relationships of this bundle with respect to every channel heads and see if it is directly/transitively replaced by those channel heads to be included as part of that channel. Without the package and rest of the bundles, all we can be certain from a single bundle is the package name.

After all, this feature is just auto-filling package name, channels, and default channels for a bundle.

This commit adds a feature to interpret bundle name and channels based on a given bundle directory and update bundle's annotation YAML. This feature is triggered when opm alpha generate/build is not supplying packageName, channels, and defaultChannels information. This Bundle directory needs to be inside a Package directory that contains all bundles. The previous reverted commit 13373cb accidentally changed opm command. This commit removes the duplicated package flag. This commit corrects bundle dir from pakcgae directory. This commit adds an error message for getting empty channels for a bundle. This commit skipes error for trying to parse anyfile as Json or Yaml format. The output directory of the annotation file is not the bundle directory rather its parent directory. This fix resolves it.

Bowenislandsong

@kevinrizza @ecordell log info added for inference.

INFO[0000] Bundle channels and package name information not provided, inferring from parent package directory 
INFO[0000] Inferred channels: beta                      
INFO[0000] Inferred package name: banzaicloud-kafka-operator 
INFO[0000] Inferred default channel: beta               
INFO[0000] Building annotations.yaml                    
INFO[0000] Building Dockerfile

Bowenislandsong · 2020-04-01T16:30:05Z

pkg/lib/bundle/generate.go

+		if packageName == "" {
+			notProvided = append(notProvided, "package name")
+		}
+		log.Infof("Bundle %s information not provided, inferring from parent package directory",


log/notification added at the info level to suggestion inference took place.

Bowenislandsong · 2020-04-01T16:30:32Z

pkg/lib/bundle/generate.go

+			if channels == "" {
+				return fmt.Errorf("error interpreting channels, please manually input channels instead")
+			}
+			log.Infof("Inferred channels: %s", channels)


The inference results are also posted.

kevinrizza · 2020-04-01T17:07:54Z

/lgtm

ecordell · 2020-04-01T17:52:13Z

/lgtm

openshift-ci-robot · 2020-04-01T17:52:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Bowenislandsong, ecordell, kevinrizza

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [ecordell,kevinrizza]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Bowenislandsong · 2020-04-01T18:25:25Z

/retest

Bowenislandsong · 2020-04-01T18:26:37Z

/hold cancel

Bowenislandsong · 2020-04-01T20:26:12Z

/retest

openshift-ci-robot requested review from exdx and gallettilance March 29, 2020 17:26

Bowenislandsong force-pushed the understand-packages branch 3 times, most recently from e99aaa9 to ed35d3d Compare March 30, 2020 01:41

openshift-ci-robot requested review from awgreene, dinhxuanvu and kevinrizza March 30, 2020 01:42

Bowenislandsong requested a review from ecordell March 30, 2020 01:43

Bowenislandsong changed the title ~~[feature] Understanding packages from directory~~ [feature] Understanding packages from bundle directory Mar 30, 2020

ecordell reviewed Mar 30, 2020

View reviewed changes

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 30, 2020

Bowenislandsong force-pushed the understand-packages branch from ed35d3d to 87200f4 Compare March 30, 2020 20:17

Bowenislandsong force-pushed the understand-packages branch from 87200f4 to 99fc008 Compare April 1, 2020 16:25

Bowenislandsong commented Apr 1, 2020

View reviewed changes

openshift-ci-robot assigned kevinrizza Apr 1, 2020

openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 1, 2020

openshift-ci-robot assigned ecordell Apr 1, 2020

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 1, 2020

openshift-merge-robot merged commit d5f8836 into operator-framework:master Apr 1, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[feature] Understanding packages from bundle directory #241

[feature] Understanding packages from bundle directory #241

Bowenislandsong commented Mar 29, 2020 •

edited

Bowenislandsong commented Mar 29, 2020

Bowenislandsong commented Mar 29, 2020

Bowenislandsong commented Mar 30, 2020 •

edited

ecordell left a comment

ecordell Mar 30, 2020

ecordell Mar 30, 2020

kevinrizza Mar 30, 2020

Bowenislandsong Mar 30, 2020

ecordell Mar 30, 2020

kevinrizza Mar 30, 2020

Bowenislandsong Mar 30, 2020

Bowenislandsong left a comment •

edited

Bowenislandsong Apr 1, 2020

Bowenislandsong Apr 1, 2020

kevinrizza commented Apr 1, 2020

ecordell commented Apr 1, 2020

openshift-ci-robot commented Apr 1, 2020

Bowenislandsong commented Apr 1, 2020

Bowenislandsong commented Apr 1, 2020

Bowenislandsong commented Apr 1, 2020

[feature] Understanding packages from bundle directory #241

[feature] Understanding packages from bundle directory #241

Conversation

Bowenislandsong commented Mar 29, 2020 • edited

Bowenislandsong commented Mar 29, 2020

Bowenislandsong commented Mar 29, 2020

Bowenislandsong commented Mar 30, 2020 • edited

ecordell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bowenislandsong left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinrizza commented Apr 1, 2020

ecordell commented Apr 1, 2020

openshift-ci-robot commented Apr 1, 2020

Bowenislandsong commented Apr 1, 2020

Bowenislandsong commented Apr 1, 2020

Bowenislandsong commented Apr 1, 2020

Bowenislandsong commented Mar 29, 2020 •

edited

Bowenislandsong commented Mar 30, 2020 •

edited

Bowenislandsong left a comment •

edited