CMP-3805: Add test for configurable access modes and Storage class (34928) #999
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)
}
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)?
| return "", fmt.Errorf("failed to get provisioner from storage class: %w", err) | ||
| } | ||
| if !found { | ||
| return "", fmt.Errorf("provisioner field not found in default storage class") |
There was a problem hiding this comment.
I wonder what this ends up being for the default storage class in our CI clusters.
There was a problem hiding this comment.
Yes, it more looks a rename here. Previously, we try to use some other storage class. But there isn't a single built-in StorageClass that works across all platforms. Only the third-party storage solutions work but additional actions needed to create it.
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
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.
|
[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 |
|
@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. |
|
/lgtm |
4 participants
To implement this PR:
One of the below is needed, or there will be an error creating Machine Config Pool e2e-invalid:
The following line needs to be commented in tests/e2e/framework/main_entry.go
line to be commented --> return fmt.Errorf("failed to create Machine Config Pool %s: %w", "e2e-invalid", err)
Use CMP-3930: Include the required selectors to machineconfig to pass the ValidatingAdmissionPolicy #960
Run it using
make e2e-parallel E2E_GO_TEST_FLAGS="-v -run TestScanWithCustomStorageClass"Expected result: