CMP-3805: Add test for configurable access modes and Storage class (34928)#999
CMP-3805: Add test for configurable access modes and Storage class (34928)#999taimurhafeez wants to merge 5 commits intoComplianceAsCode:masterfrom
Conversation
|
@taimurhafeez: This pull request references CMP-3805 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. |
|
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. |
tests/e2e/framework/main_entry.go
Outdated
| } | ||
| err = f.createInvalidMachineConfigPool("e2e-invalid") | ||
| if err != nil { | ||
| // log.Printf("continuing without invalid MachineConfigPool (cluster may have ValidatingAdmissionPolicy)") |
There was a problem hiding this comment.
I think this breaking because Printf expects a formatting directive that we're not supplying (e.g., %s).
Something like this might help:
name := "e2e-invalid"
err = f.createInvalidMachineConfigPool("e2e-invalid")
if err != nil {
log.Printf("failed to create %s MachineConfigPool: %s", name, err)
}There was a problem hiding this comment.
comment has been removed.
tests/e2e/parallel/main_test.go
Outdated
| } | ||
| } | ||
|
|
||
| // TestScanWithCustomStorageClass tests OCP-34928 |
There was a problem hiding this comment.
I asked this in the other review, but do we want to keep the downstream test IDs (OCP-34928)?
There was a problem hiding this comment.
has been removed.
tests/e2e/parallel/main_test.go
Outdated
| // Get the default storage class provisioner for our custom storage class | ||
| defaultProvisioner, err := f.GetDefaultStorageClassProvisioner() | ||
| if err != nil { | ||
| t.Fatalf("failed to get default storage class provisioner: %s", err) |
There was a problem hiding this comment.
Could this technically be considered a reason to skip this test?
There was a problem hiding this comment.
@rhmdnd After discussing with Xiaojie, I have modified the test case.
rhmdnd
left a comment
There was a problem hiding this comment.
Looks good - just one comment on if we want to consider skipping this test if the default storage class doesn't exist.
Otherwise this seems ready.
|
/lgtm |
rhmdnd
left a comment
There was a problem hiding this comment.
A couple of comments but primarily want to make sure we safely handle the unstructured error and bubble that up accordingly. Otherwise this looks good to me.
| break | ||
| } | ||
| } | ||
| // Alternative: check if the PVC name contains our scan name |
There was a problem hiding this comment.
This might not be necessary since the label should always map to the scan name (a containment check on strings will be more fragile than checking an exact match).
There was a problem hiding this comment.
If one scan is named foo and the other is foobar, depending on the order they're returned, it's possible this would return the wrong PVC, wouldn't it?
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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 |
|
@taimurhafeez: This pull request references CMP-3805 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. |
SetNestedField can return an error if it fails to set the field properly. This change updates CreateCustomStorageClass to check and return that error so tests can fail or skip appropriately rather than silently continuing with an incomplete StorageClass object.
|
🤖 To deploy this PR, run the following command: |
| break | ||
| } | ||
| } | ||
| // Alternative: check if the PVC name contains our scan name |
There was a problem hiding this comment.
If one scan is named foo and the other is foobar, depending on the order they're returned, it's possible this would return the wrong PVC, wouldn't it?
|
@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. |
Add an e2e test to verify that storage class and access modes are configurable via the ComplianceSuite and ComplianceScan specifications.
To implement this PR:
Run
make e2e-parallel E2E_GO_TEST_FLAGS="-v -run TestScanWithCustomStorageClass"Expected result on OCP 4.21: