-
Notifications
You must be signed in to change notification settings - Fork 247
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[feature] Understanding packages from directory #229
[feature] Understanding packages from directory #229
Conversation
adding test! |
pkg/sqlite/graph_loader.go
Outdated
@@ -0,0 +1,159 @@ | |||
package sqlite |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DO not review this file. This is @exdx and @gallettilance 's part
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work on this Bowen. I have some comments and questions, mostly about how we parse the input and also how we bubble up errors
cmd/opm/alpha/bundle/build.go
Outdated
@@ -55,14 +55,8 @@ func newBundleBuildCmd() *cobra.Command { | |||
} | |||
|
|||
bundleBuildCmd.Flags().StringVarP(&packageNameArgs, "package", "p", "", "The name of the package that bundle image belongs to") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO we need to update this help message to let users know that it's required if they're not pointing to a directory in the nested bundle format
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice chatch !!
pkg/lib/bundle/generate.go
Outdated
@@ -76,6 +76,24 @@ func GenerateFunc(directory, outputDir, packageName, channels, channelDefault st | |||
return err | |||
} | |||
|
|||
if channels == "" || packageName == "" || channelDefault == "" { | |||
I, err := NewBundleDirInterperter(filepath.Join(directory, "..")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: use lowercase i, err :=
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lol, right!
pkg/registry/csv.go
Outdated
@@ -53,6 +60,49 @@ type ClusterServiceVersion struct { | |||
Spec json.RawMessage `json:"spec"` | |||
} | |||
|
|||
func NewFromBundleDirectory(bundleDir string) (*ClusterServiceVersion, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we change this function name to be more descriptive? NewFromBundleDirectory describes the mechanics of what the function is doing, not the purpose or the output. Maybe ReadCSVFromBundleDirectory
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I thought about saying read
or load
, went with New because I want to say new
... somewhere... I'll change it since it makes more sense.
pkg/registry/csv.go
Outdated
} | ||
|
||
csvFile, err := func() (string, error) { | ||
for _, f := range files { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there are more clever ways we could find this file. For example we can check all the files in the directory to see if they're yaml files, and then for each yaml file attempt to serialize it into a CSV.
Otherwise this will fail if they don't follow the naming convention described here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought it is mandatory to be named that way? ... Could there be a case where there are more than one CSV file in a bundle?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is no hard requirement on the name of the file...
pkg/lib/bundle/generate.go
Outdated
if channels == "" || packageName == "" || channelDefault == "" { | ||
I, err := NewBundleDirInterperter(filepath.Join(directory, "..")) | ||
if err != nil { | ||
return fmt.Errorf("error interpretating bundle from directory %s, %v", directory, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spelling nit: interpreting
Also, it feels like the error messaging here is going to be unclear to users. Can we be more descriptive? Right now it seems like we don't differentiate between "this bundle isn't in a directory with our special helpful case" and "you didn't give us enough information to handle this request". For example if they just didn't specify a list of channels in a bundle that's in its own directory, we shouldn't surface "unable to interpret bundle from directory". we should tell them "missing input parameters".
There's also the case where a user just doesn't give us a default channel for example that we were already handling and I think this removes that ability as well
pkg/registry/directoryGraphLoader.go
Outdated
return nil, fmt.Errorf("error geting CSVs from all bundles in the package directory, %v", err) | ||
} | ||
|
||
loader.loadReplaces() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why isn't this function just part of the loader.Generate()
? It seems like this will make it harder to mock if some of the implementation is inside the constructor.
pkg/registry/directoryGraphLoader.go
Outdated
|
||
ymlFile, err := func() (string, error) { | ||
for _, f := range files { | ||
if matched, err := path.Match("*.package.yaml", f.Name()); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment as the parser for the csv, i think we can serialize this rather than rely on a naming convention
pkg/sqlite/graph_loader.go
Outdated
@@ -0,0 +1,159 @@ | |||
package sqlite |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is already merged into master with a new file name fyi: https://github.com/operator-framework/operator-registry/blob/master/pkg/sqlite/graphloader.go#L11
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, i am not sure why it shows here, I will remove it. in the next push
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's because you merged their commits into your repo and are pulling them along, but i squashed their commits and then renamed this file to get rid of the underscore in the name. you need to remove their commits and rebase.
7e2e7ab
to
8b6549f
Compare
cmd/opm/alpha/bundle/build.go
Outdated
|
||
bundleBuildCmd.Flags().StringVarP(&channelDefaultArgs, "default", "e", "", "The default channel for the bundle image") | ||
bundleBuildCmd.Flags().BoolVarP(&noChannelDefaultArgs, "no-default", "e", false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a not great user experience. We shouldn't add complexity here if it's not necessary. IMO we should just attempt to set the default channel like we were doing before if we can't find a packages.yaml file instead of asking the user if they want an error or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
? if we can not find the package.yaml then we have run the command. By then we would either successfully get the channel or that we didn't. If we got it we are writing it to the annotation file. If we didn't we would have already returned something.
what do you mean by then
we ask if users want error or not? There isn't a second step command.
I can only think of returning a warning if the default channel is not found.
Besize, we can tno interpret anything without the package.yaml file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because before we didn't attempt to look for it and tried to guess it. Why not retain that implicitly without adding an extra parameter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, can you include some unit tests?
cmd/opm/alpha/bundle/build.go
Outdated
} | ||
bundleBuildCmd.Flags().StringVarP(&channelsArgs, "channels", "c", "", | ||
"The list of channels that bundle image belongs to" + | ||
"(Required if `directory` is not pointing to a bundel in the nested bundel format)") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spelling nit: bundle
cmd/opm/alpha/bundle/build.go
Outdated
} | ||
bundleBuildCmd.Flags().StringVarP(&packageNameArgs, "package", "p", "", | ||
"The name of the package that bundle image belongs to " + | ||
"(Required if `directory` is not pointing to a bundel in the nested bundel format)") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spelling nit: bundle
cmd/opm/alpha/bundle/build.go
Outdated
bundleBuildCmd.Flags().StringVarP(&dirBuildArgs, "directory", "d", "", "The directory where bundle manifests and metadata for a specific version are located") | ||
bundleBuildCmd.Flags().StringVarP(&dirBuildArgs, "directory", "d", "", | ||
"The directory where bundle manifests and metadata for a specific version are located " + | ||
"(Can be used for interpreting bundle information if in the nested bundle format.)") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this last line is not necessary for this specific parameter.
cmd/opm/alpha/bundle/generate.go
Outdated
} | ||
bundleGenerateCmd.Flags().StringVarP(&packageNameArgs, "package", "p", "", | ||
"The name of the package that bundle image belongs to "+ | ||
"(Required if `directory` is not pointing to a bundel in the nested bundel format)") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same spelling nits as the build cmd
pkg/registry/directoryGraphLoader.go
Outdated
|
||
ymlFiles := []string{} | ||
for _, f := range files { | ||
if matched, err := path.Match("*.yaml", f.Name()); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we just check to see if it's yaml? we do something similar when we unpack bundles: https://github.com/operator-framework/operator-registry/blob/master/pkg/registry/conversion.go#L67
yep working on that now... thats why it is still wip. |
54dbef6
to
51d205d
Compare
dc7bb29
to
46afb12
Compare
if channels == "" || packageName == "" { | ||
i, err := NewBundleDirInterperter(filepath.Join(directory, "..")) | ||
if err != nil { | ||
return fmt.Errorf("please manually input channels and packageName, "+ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
|
||
decoder := yaml.NewYAMLOrJSONDecoder(csvReader, 30) | ||
if err = decoder.Decode(&unstructuredCSV); err != nil { | ||
return nil, fmt.Errorf("error unmarshalling CSV, %v", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but won't this return an error if the directory contains yaml files that aren't csvs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will name this better. This just unmarshals to unstructured. It then tries to determin if it is CSV next
if unstructuredCSV.GetKind() != operators.ClusterServiceVersionKind {
pkg/registry/csv.go
Outdated
ymlFiles := []string{} | ||
for _, f := range files { | ||
if !f.IsDir() { | ||
ymlFiles = append(ymlFiles, f.Name()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't we use the yaml decoder here to see if the file is actually a yaml file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This just grabs all files. I will rename the variables. This gets all files, then it unmarshals to yaml. then it tries to see if it is csv.
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.
46afb12
to
38fdca0
Compare
@Bowenislandsong: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Bowenislandsong, 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:
Approvers can indicate their approval by writing |
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 supplyingpackageName
,channels
, anddefaultChannels
information.This Bundle directory needs to be inside a Package directory that contains all bundles.