Add package for handling bundle formats #396
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
There was a problem hiding this comment.
I took a glance at these changes and they seem relatively reasonable to me. I'm a fan of using the options pattern here to help improve interacting with this library.
I have a couple of quick questions:
- Can we create a tracking issue for modularizing this library?
- Does it make sense to have a standalone commit for moving the convert library from the internal directory to the pkg directory?
I think my only concern right now is support guarantees with exported functions in a pkg/ directory that clients depend on while we continue to iterate on this project. Maybe it's sufficient in the short term to have a nested README that adds a warning that the exported functions/behaviors are subject to change? Thoughts?
As long as we stay at v0, we're good to make breaking changes on minor version bumps of this repo. |
Yeah, this seems super reasonable to me. Does the registry code around FBCs also have an intermediate representation layer too? |
openshift-ci
bot
added
the
needs-rebase
openshift-ci
bot
removed
the
needs-rebase
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
pkg/bundle/plain/v0/bundle.go
Outdated
| } | ||
|
|
||
| func (b Bundle) generateName(base string, o interface{}) string { | ||
| const maxNameLength = 63 |
There was a problem hiding this comment.
nit: we enforce that the length of a rukpak Bundle type is maximum 40 characters long (see https://github.com/operator-framework/rukpak/blob/main/manifests/apis/crds/patches/bundle_validation.yaml#L1). Would it make sense to have a similar limit here?
There was a problem hiding this comment.
Pinging @joelanford and @timflannagan to weigh on this one.
Co-authored-by: exdx <dsover@redhat.com>
…pak into feature/public-convert-lib
ryantking
changed the title
| // TODO(ryantking): Should we support any other file system abstractions beyond `fs.FS`? | ||
| // `fs.FS` doesn't support any write operations. They can modify the objects in an `ObjectFile`, but that won't change |
There was a problem hiding this comment.
IMO, fs.FS is sufficient. We can use https://pkg.go.dev/testing/fstest#MapFS as a writable in-memory FS implementation, and if we want a writable disk implementation, then we can just use os.* functions to create the necessary files/directories and then use os.DirFS to read it.
| // WithManifestDir returns an option for creating new bundle filesystems. | ||
| // | ||
| // This option specifies which directories can contain manifests. If this opion is unset then the filesystem will search | ||
| // in all directories for manifests. | ||
| func WithManifestDirs(dirs ...string) func(*FS) { | ||
| return func(fsys *FS) { | ||
| fsys.manifestDirs = append(fsys.manifestDirs, dirs...) | ||
| } | ||
| } |
There was a problem hiding this comment.
Is this too bundle format specific? A helm chart bundle format wouldn't have a manifests directory at all. How would that type of bundle work with this package?
| if filepath.Ext(name) != ".yaml" || filepath.Ext(name) == ".yml" { | ||
| return false | ||
| } |
There was a problem hiding this comment.
Is there a bug here with != and ==? Also, what about json files?
| // TODO(ryantking): Does srcBundle.CSV().GetName() give us package name | ||
| // per @joelanford's install namespace detection suggestion? | ||
| b.installNamespace = fmt.Sprintf("%s-system", csv.GetName()) |
There was a problem hiding this comment.
No, csv.GetName() will return the CSV name (typically something like foo.v0.1.0). We want this to be the package name (typically something like foo).
The package name can be discovered from a registry+v1 bundle's metadata/annotations.yaml file.
| return Bundle{bundleFS} | ||
| } | ||
|
|
||
| return Bundle{bundle.New(fsys, bundle.WithManifestDirs("manifests"))} |
There was a problem hiding this comment.
Technically the manifests directory for a registry+v1 bundle is defined in a property in the metadata/annotations.yaml file. I don't think I've ever seen a bundle image where that value is anything other than /manifests, so I think this is probably fine.
But just an FYI, that there may eventually be a bug reported against this functionality for not respecting the manifests dir defined in the annotations file.
| // embed.FS is only walkable from the root directory so this function accepts a prefix to filter out unwanted files. | ||
| // This looks like it could be a go bug if its not explicitly called out. |
There was a problem hiding this comment.
Can you use https://pkg.go.dev/io/fs#Sub here? It returns an fs that is a subdirectory of the root FS.
| if len(fsys.manifestDirs) == 0 { | ||
| return true | ||
| } |
There was a problem hiding this comment.
In the case of a theoretical helm chart bundle, I assume there would be no manifest dirs, which would mean every file is a manifest file? Should this block actually return false?
| for _, dir := range fsys.manifestDirs { | ||
| if strings.HasPrefix(name, dir) { | ||
| return true | ||
| } | ||
| } |
There was a problem hiding this comment.
This seems like something that should be left to the bundle specification. For explicitness and clarity, we said that plain manifest bundles:
- must have a
manifestsdirectory in the root of the bundle - must not have any subdirectories in the
manifestsdirectory (i.e. themanifestsdirectory must be a flat list of manifest files.
However seems like it would parse manifest files in subdirectories of a manifests dir. I think this gets back to my comment that perhaps the generic bundle code should not have any concept of manifests or kubernetes objects?
How much would it blow up this PR if we said that we wouldn't parse k8s objects at all in this generic bundle.FS layer? Would there be anything valuable left?
This adds package
github.com/operator-framework/rukpak/pkg/bundle, which is a public API for working with and converting between bundle formats. Support is limited for theregistry+v1andplain+v0formats and converting from the former to the latter.