Add support for declarative configs to "adm catalog mirror" #868
Conversation
Signed-off-by: Joe Lanford <joe.lanford@gmail.com>
|
/test e2e-metal-ipi-ovn-ipv6 |
1 similar comment
|
/test e2e-metal-ipi-ovn-ipv6 |
pkg/cli/admin/catalog/mirror.go
Outdated
| } | ||
| return err | ||
| } | ||
| if blob.Schema != "olm.bundle" { |
There was a problem hiding this comment.
we might get some nice future-proofing by just checking for .Image or .RelatedImages and adding if they exist (so we don't have to update this when we have olm.bundle.v2 or olm.staticbundles etc)
Signed-off-by: Joe Lanford <joe.lanford@gmail.com>
|
/retest |
1 similar comment
|
/retest |
|
/assign @mfojtik |
|
/lgtm |
|
/approve |
| func (_ declcfgRelatedImagesParser) Parse(root string) (map[string]struct{}, error) { | ||
| rootFS := os.DirFS(root) | ||
|
|
||
| matcher, err := ignore.NewMatcher(rootFS, ".indexignore") |
There was a problem hiding this comment.
If this is the only call for the matcher I don't see any reason why we want to include 2 more packages, which of one is single-method helper and another is a transient dependency. Can't this be replaced with a simple file matcher?
There was a problem hiding this comment.
In github.com/operator-framework/operator-registry, we decided to use the .gitignore style file matching for two reasons:
- Almost all users are familiar with it
- It supports multiple ignore files at multiple levels in a directory structure, which is important with declarative config based indexes for composability reasons.
Regardless of the choice of matcher, the matching semantics need to be consistent between opm and oc.
One alternative would be to inline the joelanford/ignore library directly into oc. I made that library as a simple shim over go-git's gitignore library.
There was a problem hiding this comment.
Can you verify if one of the pre-existing libraries already included in oc doesn't have identical functionality, quick grep lead me to https://github.com/openshift/oc/blob/master/vendor/github.com/monochromegane/go-gitignore/gitignore.go although I haven't checked its functionality
There was a problem hiding this comment.
From https://github.com/monochromegane/go-gitignore#features
go-gitignore use filepath.Match for matching meta char pattern, so not support recursive pattern (path/**/file).
Also, it doesn't appear to have built-in support for building a hierarchical matcher based on multiple gitignore files found in a directory tree, so we'd have to implement/test/maintain that as a wrapper library somewhere anyway, I think.
Looks like monochromegane/go-gitignore is brought in by https://github.com/openshift/oc/blob/master/vendor/sigs.k8s.io/kustomize/kyaml/kio/ignorefilesmatcher.go#L11, so not something we could directly control and switch over to go-git's gitignore library.
There was a problem hiding this comment.
Also, it doesn't appear to have built-in support for building a hierarchical matcher based on multiple gitignore files found in a directory tree, so we'd have to implement/test/maintain that as a wrapper library somewhere anyway, I think.
I'd like to see that verified do you actually need this. I skimmed through the proposal and it wasn't explicitly obvious that you do.
Looks like monochromegane/go-gitignore is brought in by https://github.com/openshift/oc/blob/master/vendor/sigs.k8s.io/kustomize/kyaml/kio/ignorefilesmatcher.go#L11, so not something we could directly control and switch over to
go-git's gitignore library.
yeah, I wasn't proposing switching that in kustomize's kyaml 😉
One alternative would be to inline the
joelanford/ignorelibrary directly intooc. I made that library as a simple shim over go-git's gitignore library.
yeah, if anything I'd prefer do it this way.
There was a problem hiding this comment.
I'd like to see that verified do you actually need this. I skimmed through the proposal and it wasn't explicitly obvious that you do.
We do need this. One of the design ideas (not explicitly covered in the proposal) is that multiple DC-based indexes can be combined into a composite index. So for example, I can have the following, where indexComposite, indexA, indexB, pkg1, and pkg2 are all valid indexes:
$ tree -a indexComposite
indexComposite
├── DOC.md
├── indexA
│ ├── .indexignore
│ └── index.yaml
├── indexB
│ ├── .indexignore
│ ├── pkg1
│ │ ├── .indexignore
│ │ └── index.json
│ ├── pkg2
│ │ ├── .indexignore
│ │ └── index.yaml
│ └── README.md
└── .indexignore
The idea is that operator authors will be able to build a single-package index (e.g. pkg1 and pkg2), and catalog maintainers will be able to build indexes containing multiple packages and/or other indexes (e.g. indexComposite, indexA, and indexB). Each index maintainer should be able to include a .indexignore file in their index such that when their index is included in another index, their ignore patterns still apply, and apply in their index only without affecting other sibling or parent indexes.
yeah, if anything I'd prefer do it this way.
I can update the PR to avoid the joelanford/ignore dependency. The only concern I have with this is that oc and opm will be slightly harder to keep in sync. In the current setup, I would make a change in joelanford/ignore and then bump go.mod files. If I inline the code here, what was a go.mod bump becomes a copy/paste of whatever changed.
I don't anticipate changes to joelanford/ignore often (if ever), so probably no big deal, but I wanted to call this out at least to put all my cards on the table.
openshift-ci
bot
added
the
do-not-merge/hold
openshift-ci
bot
removed
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ecordell, joelanford, soltysh 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 |
openshift-ci
bot
added
the
approved
See https://github.com/operator-framework/enhancements/blob/master/enhancements/declarative-index-config.md for background about OLM's declarative config indexes.
Signed-off-by: Joe Lanford joe.lanford@gmail.com