USHIFT-647: skip non-existing resources from security.openshift.io #27615
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: pacevedom 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 |
|
@pacevedom: The following tests 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. |
| exist, err := exutil.DoesApiResourceExist(oc.AdminConfig(), bt.Resource, groupName) | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
| if !exist { | ||
| e2e.Logf("Resource %s does not exist, skipping", bt) |
There was a problem hiding this comment.
This will show the test as passed, not skipped. This needs to be g.Skip instead.
There was a problem hiding this comment.
Can't skip the whole test, as this is only skipping specific resources from an API group. Using g.Skip would mean skipping the entire API group.
There was a problem hiding this comment.
Which is the expected behavior. Unless, the g.It gets moved on the level of individual resources. The test is designed to fail as a single apigroup. With the continue left the test will be seen as passed where it should be skipped or failed.
An API group is either installed or not installed. We do not assume existince/non-existence of individual resource inside a single api group.
There was a problem hiding this comment.
If the behaviour of the test is to fail/skip as a single apigroup, then we need to use g.Skip instead. @pacevedom WDYT?
There was a problem hiding this comment.
The test is expected to test entire api group as a whole. If either of the resources is missing, an api group is not considered as fully tested. Which will break the original assumption of testing all the enumerated resources.
It is questionable whether an api group can be only partially tested. That would require properly checking each resource in each apigroup. Which is something that has not been done so far IINM. If the goal is to only test a subset of security.openshift.io group for MicroShift, then I would suggest to consider breaking this specific api group into two of the same group just different list of resources (as long as it make sense). Or figuring out a different way how to partially test this group. E.g. introducing a new test just for it.
There was a problem hiding this comment.
@ingvagabund I checked that all the three resources present under "security.openshift.io" apigroup are not present for MicroShift. What if we skip this entire apigroup because it's applicable for this apigroup only ? Something like
if !exist && groupName == "security.openshift.io" {
g.Skip(fmt.Sprintf("Resource %s does not exist, skipping", bt.Resource))
}This will avoid partial testing of other apigroups.
There was a problem hiding this comment.
https://github.com/openshift/microshift/blob/main/docs/enabled_apis.md mentions SecurityContextConstraints of security.openshift.io/v1
There was a problem hiding this comment.
Added a commit here, which skips only security.openshift.io group if any of its resources are not present.
Then added a new test which checks the resources applicable to MicroShift (e.g SecurityContextConstraints)
Please let me know your thoughts on this.
| exist, err := exutil.DoesApiResourceExist(oc.AdminConfig(), resource[0], groupName) | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
| if !exist { | ||
| e2e.Logf("Resource %s does not exist, skipping", gvr) |
There was a problem hiding this comment.
This will show the test as passed, not skipped. This needs to be g.Skip instead.
There was a problem hiding this comment.
Can't skip the whole test, as this is only skipping specific resources from an API group. Using g.Skip would mean skipping the entire API group.
|
@ingvagabund @pacevedom , what's the status of this work? |
|
#27615 (comment) waiting for resolution |
|
I've opened #27897 PR to address this one. |
|
/close #27897 PR got merged, so it's obsolete now. |
|
@chiragkyal: Closed this PR. 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. |
/cc @ingvagabund