Bug 1868125: [opm] Add valid bundles to index when permissive enabled
#517
Conversation
When the `opm index add` was used with the `permissive` flag, the index was being created, but the valid bundles from the list of bundles provided with the command was not being added. Instead, an empty index was being created. This PR fixes the issue by adding the valid bundles to the index, while ignoring the invalid ones.
openshift-ci-robot
added
the
bugzilla/severity-low
|
@anik120: This pull request references Bugzilla bug 1868125, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
In response to this:
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. |
openshift-ci-robot
added
the
bugzilla/valid-bug
|
/lgtm |
| } | ||
|
|
||
| func NewDirectoryPopulator(loader Load, graphLoader GraphLoader, querier Query, imageDirMap map[image.Reference]string, overwriteDirMap map[string]map[image.Reference]string, overwrite bool) *DirectoryPopulator { | ||
| func NewDirectoryPopulator(loader Load, graphLoader GraphLoader, querier Query, imageDirMap map[image.Reference]string, overwriteDirMap map[string]map[image.Reference]string, overwrite, permissive bool) *DirectoryPopulator { |
There was a problem hiding this comment.
I don't think we need to propagate this down so long as we always aggregate errors and continue instead of exiting immediately.
There was a problem hiding this comment.
@njhale I'm trying to think why we might prefer to aggregate errors instead of propagating it all the way down.
The reason I originally decided to take it all the way down is coz
a) If we have introduced a concept of permissive for opm it seems natural to have the same concept for our DirectoryPopulator too. Basically if some other implementation instead of opm want to use DirectoryPopulator it should be aware/have access to permissive too.
b) If you scale this to n bundles where n is large, it seems more efficient to fail fast if permissive isn't enabled instead of churning all the way through.
There was a problem hiding this comment.
It seems like the error-permissive concept would benefit from a non-permissive error type, since there are error instances that shouldn't be permitted in any case.
type ErrNonPermissive struct {
err error
}
func foo(l string) error {
if err = nonPermissiveErrorReturner(l); err != nil {
return ErrNonPermissive{err}
}
if err = permissiveErrorReturner(l); err != nil {
return err
}
return nil
}
func bar(list []string, permissive bool) error {
var errs []error
for _, l := range list {
err := foo(l)
if err != nil {
if errors.As(err, &ErrNonPermissive{}) || !permissive {
return err
}
errs = append(errs, err)
}
}
return utilerrors.NewAggregate(errs)
}Then you wouldn't have to propagate permissive down as far, and you could have any procedure that returns an error "know" about permissiveness without implementing (too much) of the handling logic itself
There was a problem hiding this comment.
If you scale this to n bundles where n is large, it seems more efficient to fail fast if permissive isn't enabled instead of churning all the way through.
fair enough, but these function signatures are already unwieldy as they are today. For the most part, I think we could benefit from not copying all this data around by making populate a method of DirectoryPopulator (unless it's used by more than one type of populator).
I'm also of the opinion that a lot of this code will get tossed soon anyway, so it may not be super important to keep things pretty now, but we should definitely think about it for the next iteration of operator-registry.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: anik120 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| remainingImages = append(remainingImages, pkgImage) | ||
| if !i.permissive { | ||
| pkgRemainingImages++ | ||
| remainingImages = append(remainingImages, pkgImage) |
There was a problem hiding this comment.
Are remainingImages not used by callers who want permissive population?
There was a problem hiding this comment.
@estroz remainingImages are the pkgImage s that are not "valid bundles" and could not be added to foundImages. The reason we need to not add "invalid bundles" to remainingImages when permissive mode is enabled is to make sure that we don't loop infinitely because imagesToAdd always has those "invalid images" in them
|
/retest |
1 similar comment
|
/retest |
|
@anik120: The following test failed, say
Full PR test history. Your PR dashboard. 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. |
openshift-ci-robot
added
the
needs-rebase
|
@anik120: PR needs rebase. 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. |
|
@anik120: This pull request references Bugzilla bug 1868125. The bug has been updated to no longer refer to the pull request using the external bug tracker. All external bug links have been closed. The bug has been moved to the NEW state. In response to this:
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. |
Description of the change:
When the
opm index addwas used with thepermissiveflag, the indexwas being created, but the valid bundles from the list of bundles provided
with the command was not being added. Instead, an empty index was being
created. This PR fixes the issue by adding the valid bundles to the index,
while ignoring the invalid ones.
Motivation for the change:
Reviewer Checklist
/docs