API-1835: manifestclient: allow listing only cluster-scoped-resources #1888
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@p0lyn0mial: This pull request references API-1835 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target the "4.18.0" version, but no target version was set. 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 openshift-eng/jira-lifecycle-plugin repository. |
|
@p0lyn0mial: This pull request references API-1835 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target the "4.18.0" version, but no target version was set. 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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
bot
added
the
approved
pkg/manifestclient/list.go
Outdated
| @@ -241,7 +241,7 @@ func allPossibleListFileLocations(sourceFS fs.FS, requestInfo *apirequest.Reques | |||
|
|
|||
| namespaces, err := allNamespacesWithData(sourceFS) | |||
| if err != nil { | |||
There was a problem hiding this comment.
is this a not found error in your case? If so, how about we ignore only that error.
There was a problem hiding this comment.
eh, it turns out that we already have a test that lists cluster-scoped-resources without the namespace directory.
The test works because cluster-scoped-resources are expected to be nested in a directory.
For example: cluster-scoped-resources/operator.openshift.io/authentications/cluster.yaml
NOT: cluster-scoped-resources/operator.openshift.io/authentications.yaml
I copy-pasted the directory structure from MOM, which apparently requires a fix. I will issue a separate PR for that.
There was a problem hiding this comment.
heh, okay, actually both are valid and can be found in a must-gather dir.
p0lyn0mial
force-pushed
the
manifest-client-improve-list
branch
from
809d809
to
5983fd3
Compare
|
@p0lyn0mial: all tests passed! 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-sigs/prow repository. I understand the commands that are listed here. |
| switch { | ||
| case errors.Is(err, fs.ErrNotExist): | ||
| return allPossibleListFileLocations, nil | ||
| case err != nil: |
There was a problem hiding this comment.
Makes sense to me. So IIRC this branch takes all possible file locations for all resources, namespaced or not.
Previously, we scraped cluster-scoped-resources and then tried to scrape all existing namespace directories. If the namespace directories didn’t exist, it would throw an error and stop. Now, it just returns all the file locations it knows about so far without failing.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bertinatto, p0lyn0mial 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 |
a best effort to allowlist only cluster-scoped resources.
xref: openshift/multi-operator-manager#49