CMP-3800: Add e2e test (33431) for ComplianceCheckResult label queries#981
Conversation
|
Hi @taimurhafeez. Thanks for your PR. I'm waiting for a github.com member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
|
Hi @taimurhafeez, I don't think it is meaningful to create a dedicate serial test case to check the labels for the ccr. If you really want to cover the test point, I think it is better to adding label queries to 1 existing parallel tests than creating a dedicated serial test |
| // NodeSelector: workerNodesLabel, | ||
| // ComplianceScanSettings: compv1alpha1.ComplianceScanSettings{ | ||
| // Debug: true, | ||
| // }, |
There was a problem hiding this comment.
I would keep the curly bracket where it originally was, so that we do not merge changes that are not related to the test case
There was a problem hiding this comment.
Just nitpicking with the prevous comment and here as well - the same applies to the commented lines in func createInvalidMachineConfigPool in tests/e2e/framework/common.go and the comments in tests/e2e/framework/main_entry.go
I know it is early in the reviews, but I think it would be best to have it prepared exactly the way we would like to merge it :)
|
The test case is PASSing succesfully on OCP 4.20 cluster. After running it, one of the worker nodes on my cluster has gone into degraded state though: I am not really sure if this is just a coincidence or what is a rootcase yet, so far I just want to post it here so we have a trace that this has happened - in case it happens to someone else as well in the future |
|
@taimurhafeez: This pull request references CMP-3800 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 story to target the "4.21.0" version, but no target version was set. DetailsIn 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. |
|
|
|
Potential transient issue: |
possibly because I ran recently, and it went through |
|
/retest |
|
| f := framework.Global | ||
| bindingName := framework.GetObjNameFromTest(t) | ||
| tpName := framework.GetObjNameFromTest(t) | ||
|
|
There was a problem hiding this comment.
Unrelated whitespace change?
| Extends: "ocp4-moderate", | ||
| }, | ||
| } | ||
|
|
There was a problem hiding this comment.
Unrelated whitespace change?
| if err := f.WaitForSuiteScansStatus(f.OperatorNamespace, bindingName, compv1alpha1.PhaseDone, compv1alpha1.ResultNonCompliant); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
|
|
There was a problem hiding this comment.
Unrelated whitespace change?
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
|
|
There was a problem hiding this comment.
Unrelated whitespace change?
rhmdnd
left a comment
There was a problem hiding this comment.
Only minor suggestions inline. Otherwise this is looking good.
@rhmdnd Regarding whitespaces, I was addressing Xiaoji's comment about removing leftover lines, and then I thought it was better to keep them minimized. But now I have addressed this. |
|
/retest |
rhmdnd
left a comment
There was a problem hiding this comment.
Sorry for taking so long to get back to this. I have a couple recommendations on how we can make the assertions a little more robust, and a minor whitespace comment.
|
|
||
| return nil | ||
| } | ||
|
|
There was a problem hiding this comment.
Unrelated whitespace change?
|
@taimurhafeez: This pull request references CMP-3800 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 story to target the "4.22.0" version, but no target version was set. DetailsIn 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. |
Thanks, Lance, for taking the time to review and useful comments. I have tried to address in my recent commits. |
|
🤖 To deploy this PR, run the following command: |
rhmdnd
left a comment
There was a problem hiding this comment.
/lgtm
/approve
Thanks for addressing all the comments here. Looks great.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rhmdnd, taimurhafeez The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
ROSA and serial test failures are unrelated. Attempting a fix for the serial failure (which appears to be transient) in #1100. |
Verify that ComplianceCheckResult objects can be queried using labels: - compliance.openshift.io/check-severity - compliance.openshift.io/check-status - compliance.openshift.io/scan-name - compliance.openshift.io/suite Add AssertCheckResultByLabel helper to test framework
…nes 3) placed a misplaced bracket 4)Consolidated repetitive label verification code into a single loop structure
…stead of just first and used lighter profile e8
70bd7e0 to
2341083
Compare
|
New changes are detected. LGTM label has been removed. |
|
🤖 To deploy this PR, run the following command: |
|
@taimurhafeez: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
|
Serial tests are failing becuase of This PR only modified the parallel tests, which passed cleanly on both aarch64 and amd64. |
Verify that ComplianceCheckResult objects can be queried using labels:
Changes
TestScanProducesRemediationstoTestScanProducesRemediationsAndLabelsintests/e2e/parallel/main_test.goAssertCheckResultByLabelhelper intests/e2e/framework/common.gomake e2e-parallel E2E_GO_TEST_FLAGS="-v -run TestScanProducesRemediationsAndLabels"Tested on OCP 4.21 and expected output: