WIP: Add test verifies that OLMv1 does not revert user-initiated changes to deployed resources#2512
WIP: Add test verifies that OLMv1 does not revert user-initiated changes to deployed resources#2512camilamacedo86 wants to merge 1 commit intooperator-framework:mainfrom
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Pull request overview
This PR adds an end-to-end test to verify that OLMv1, when using Server-Side Apply, does not revert user-initiated changes to deployed resources. The test specifically validates the scenario where a user runs kubectl rollout restart deployment, which was problematic in OLMv0. The PR is explicitly marked as Work In Progress (WIP).
Changes:
- Added new feature file
rollout-restart.featurewith a scenario testing user-initiated deployment changes - Implemented three new step functions:
UserAddsRestartAnnotation,ResourceHasRestartAnnotation, andClusterExtensionAnnotationIsSet
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| test/e2e/features/rollout-restart.feature | New feature file defining the test scenario for verifying OLMv1 doesn't revert user-initiated changes to deployments |
| test/e2e/steps/steps.go | Added three new step functions to support the rollout restart test scenario and registered them in the step definitions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
test/e2e/steps/steps.go
Outdated
| parts := strings.Split(resourceName, "/") | ||
| if len(parts) != 2 { | ||
| return fmt.Errorf("invalid resource name format: %s (expected kind/name)", resourceName) | ||
| } | ||
| kind := parts[0] |
There was a problem hiding this comment.
Use strings.Cut instead of strings.Split for parsing resource names in the format "kind/name". This is the established pattern used consistently throughout this codebase, as seen in functions like ResourceAvailable (line 535), ResourceRemoved (line 549), and ResourceEventuallyNotFound (line 569). The strings.Cut function is more appropriate here as it returns a boolean indicating whether the separator was found, allowing for clearer error handling.
| parts := strings.Split(resourceName, "/") | |
| if len(parts) != 2 { | |
| return fmt.Errorf("invalid resource name format: %s (expected kind/name)", resourceName) | |
| } | |
| kind := parts[0] | |
| kind, _, ok := strings.Cut(resourceName, "/") | |
| if !ok { | |
| return fmt.Errorf("invalid resource name format: %s (expected kind/name)", resourceName) | |
| } |
| func ResourceHasRestartAnnotation(ctx context.Context, resourceName string) error { | ||
| sc := scenarioCtx(ctx) | ||
|
|
||
| // Parse the resource name (format: "deployment/test-operator") | ||
| parts := strings.Split(resourceName, "/") | ||
| if len(parts) != 2 { | ||
| return fmt.Errorf("invalid resource name format: %s (expected kind/name)", resourceName) | ||
| } | ||
| kind := parts[0] | ||
| deploymentName := parts[1] | ||
|
|
||
| if kind != "deployment" { | ||
| return fmt.Errorf("only deployment resources are supported for restart annotation check, got: %s", kind) | ||
| } | ||
|
|
||
| // Check for the restart annotation added by kubectl rollout restart | ||
| restartAnnotationKey := "kubectl.kubernetes.io/restartedAt" | ||
|
|
||
| waitFor(ctx, func() bool { | ||
| // Get the restart annotation from the deployment's pod template | ||
| out, err := k8sClient("get", "deployment", deploymentName, "-n", sc.namespace, | ||
| "-o", fmt.Sprintf("jsonpath={.spec.template.metadata.annotations['%s']}", restartAnnotationKey)) | ||
| if err != nil { | ||
| return false | ||
| } | ||
|
|
||
| // If the annotation exists and has a value, it persisted | ||
| return out != "" | ||
| }) | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
The function should apply substituteScenarioVars to the resourceName parameter, consistent with other similar functions in this codebase such as ResourceAvailable, ResourceRemoved, and ResourceEventuallyNotFound. This ensures that template variables are properly substituted before parsing.
test/e2e/steps/steps.go
Outdated
| parts := strings.Split(resourceName, "/") | ||
| if len(parts) != 2 { | ||
| return fmt.Errorf("invalid resource name format: %s (expected kind/name)", resourceName) |
There was a problem hiding this comment.
Use strings.Cut instead of strings.Split for parsing resource names in the format "kind/name". This is the established pattern used consistently throughout this codebase, as seen in functions like ResourceAvailable (line 535), ResourceRemoved (line 549), and ResourceEventuallyNotFound (line 569).
| func UserAddsRestartAnnotation(ctx context.Context, resourceName string) error { | ||
| sc := scenarioCtx(ctx) | ||
|
|
||
| // Parse the resource name (format: "deployment/test-operator") | ||
| parts := strings.Split(resourceName, "/") | ||
| if len(parts) != 2 { | ||
| return fmt.Errorf("invalid resource name format: %s (expected kind/name)", resourceName) | ||
| } | ||
| kind := parts[0] | ||
|
|
||
| if kind != "deployment" { | ||
| return fmt.Errorf("only deployment resources are supported for restart annotation, got: %s", kind) | ||
| } | ||
|
|
||
| // Use kubectl rollout restart to add the restart annotation | ||
| // This is the actual command users would run, ensuring we test real-world behavior | ||
| _, err := k8sClient("rollout", "restart", resourceName, "-n", sc.namespace) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to rollout restart %s: %w", resourceName, err) | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
The function should apply substituteScenarioVars to the resourceName parameter, consistent with other similar functions in this codebase such as ResourceAvailable, ResourceRemoved, and ResourceEventuallyNotFound. This ensures that template variables like ${TEST_NAMESPACE} are properly substituted.
…o deployed resources
camilamacedo86
force-pushed
the
test-asked
branch
from
6173613 to
6893e06
Compare
1 participant
No description provided.