Migrate test cases to Project API testing KubeAPI server functionality#31310
Migrate test cases to Project API testing KubeAPI server functionality#31310YamunadeviShanmugam wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds four new serialized Ginkgo e2e tests covering invalid node selector rejection, node selector display via ChangesProject API E2E Tests
Sequence Diagram(s)sequenceDiagram
participant Test
participant ProjectAPI as Project<br/>API
participant ConfigAPI as Cluster<br/>Config
participant APIServerOp as OpenShift<br/>APIServer Operator
participant ClusterOp as Cluster<br/>Operator
rect rgba(200, 220, 255, 0.5)
Note over Test,ConfigAPI: Template Propagation Test
Test->>ConfigAPI: PATCH project.config.openshift.io/cluster<br/>set projectRequestTemplate
Test->>APIServerOp: extract observed projectRequestTemplate
Test->>ClusterOp: poll conditions: Available,<br/>NotProgressing, NotDegraded
Test->>ProjectAPI: create project post-update
ProjectAPI-->>Test: namespace contains injected<br/>LimitRange and ResourceQuota
end
rect rgba(200, 255, 200, 0.5)
Note over Test,ProjectAPI: Cascading Deletion Test
Test->>ProjectAPI: create project with pods,<br/>configmaps, secrets
Test->>ProjectAPI: DELETE project namespace
Test->>ProjectAPI: poll namespace deletion,<br/>verify resources removed
Test->>ProjectAPI: recreate project name
ProjectAPI-->>Test: verify namespace empty,<br/>no leftover resources
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 12 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (12 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/extended/project/project.go (2)
764-784: 💤 Low valueRefactor: Use the helper function to avoid duplication.
This polling logic duplicates the
waitForOpenShiftAPIServerOperatorStableWithPollinghelper function defined later in this file (lines 967-991). Consider reusing the helper.♻️ Suggested refactor
- err = wait.PollUntilContextCancel(ctx, 30*time.Second, true, func(ctx context.Context) (bool, error) { - co, err := oc.AdminConfigClient().ConfigV1().ClusterOperators().Get(ctx, "openshift-apiserver", metav1.GetOptions{}) - if err != nil { - return false, nil - } - var available, progressing, degraded bool - for _, c := range co.Status.Conditions { - if c.Type == configv1.OperatorAvailable { - available = c.Status == configv1.ConditionTrue - } else if c.Type == configv1.OperatorProgressing { - progressing = c.Status == configv1.ConditionTrue - } else if c.Type == configv1.OperatorDegraded { - degraded = c.Status == configv1.ConditionTrue - } - } - if degraded { - return false, fmt.Errorf("openshift-apiserver operator is degraded") - } - return available && !progressing, nil - }) + err = waitForOpenShiftAPIServerOperatorStableWithPolling(ctx, oc)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/extended/project/project.go` around lines 764 - 784, The polling logic for checking the OpenShift API server operator status is duplicated inline in the code block starting at line 764. Remove the inline wait.PollUntilContextCancel block that checks cluster operator conditions (available, progressing, degraded) and replace it with a call to the existing helper function waitForOpenShiftAPIServerOperatorStableWithPolling which is defined later in the file and contains the same polling logic. This will eliminate the code duplication and improve maintainability.
884-895: ⚡ Quick winConsider detecting permanent pod failures to fail fast.
The pod readiness check only detects transitional states (
ContainerCreating,Init,Pending). If the image pull fails permanently (e.g.,ImagePullBackOff,ErrImagePull) or the container crashes (CrashLoopBackOff), the test will wait the full 3 minutes before timing out.♻️ Suggested improvement
err = wait.PollUntilContextTimeout(context.Background(), 10*time.Second, 3*time.Minute, false, func(ctx context.Context) (bool, error) { podOutput, err := oc.AsAdmin().WithoutNamespace().Run("get").Args("pods", "-n", projectName, "--no-headers").Output() if err != nil { return false, nil } + // Fail fast on permanent errors + if matched, _ := regexp.MatchString(`(ImagePullBackOff|ErrImagePull|CrashLoopBackOff|Error)`, podOutput); matched { + return false, fmt.Errorf("pod entered failure state: %s", podOutput) + } if matched, _ := regexp.MatchString(`(ContainerCreating|Init|Pending)`, podOutput); matched { return false, nil } return strings.TrimSpace(podOutput) != "", nil })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/extended/project/project.go` around lines 884 - 895, The pod readiness check in the wait.PollUntilContextTimeout function with the regexp.MatchString call only detects transitional states (ContainerCreating, Init, Pending) but does not check for permanent failure states like ImagePullBackOff, ErrImagePull, or CrashLoopBackOff. Add an additional regexp.MatchString check within the same polling condition to detect these failure states, and return false immediately when such a state is found so the wait fails fast instead of continuing for the full 3-minute timeout duration.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/extended/project/project.go`:
- Around line 909-916: The deletion verification in the poll function currently
treats any error from the oc get project command as successful deletion by
returning true when getErr is not nil. This masks transient errors like network
or authentication problems. Instead of immediately returning true when getErr is
not nil, check if the error message specifically contains "not found" text using
regexp.MatchString (similar to how it's already checking the output), and only
return true if it matches that specific pattern. For other errors, either return
false to continue polling or propagate the actual error to distinguish deletion
from transient failures.
---
Nitpick comments:
In `@test/extended/project/project.go`:
- Around line 764-784: The polling logic for checking the OpenShift API server
operator status is duplicated inline in the code block starting at line 764.
Remove the inline wait.PollUntilContextCancel block that checks cluster operator
conditions (available, progressing, degraded) and replace it with a call to the
existing helper function waitForOpenShiftAPIServerOperatorStableWithPolling
which is defined later in the file and contains the same polling logic. This
will eliminate the code duplication and improve maintainability.
- Around line 884-895: The pod readiness check in the
wait.PollUntilContextTimeout function with the regexp.MatchString call only
detects transitional states (ContainerCreating, Init, Pending) but does not
check for permanent failure states like ImagePullBackOff, ErrImagePull, or
CrashLoopBackOff. Add an additional regexp.MatchString check within the same
polling condition to detect these failure states, and return false immediately
when such a state is found so the wait fails fast instead of continuing for the
full 3-minute timeout duration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 560f4189-9491-4d9a-a6c7-4fde004881de
📒 Files selected for processing (3)
test/extended/project/project.gotest/extended/testdata/bindata.gotest/extended/testdata/project/project-request-limits-quota.yaml
YamunadeviShanmugam
force-pushed
the
OTE_migrate_apiserver
branch
from
26ba874 to
e642a80
Compare
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
Scheduling required tests: |
YamunadeviShanmugam
force-pushed
the
OTE_migrate_apiserver
branch
from
e642a80 to
bc9b7f9
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/extended/project/project.go`:
- Around line 742-746: The function
openshiftAPIServerObservedProjectRequestTemplate uses context.Background()
internally, which prevents the surrounding poll timeout from cancelling stuck
API reads. Modify the openshiftAPIServerObservedProjectRequestTemplate function
signature to accept a context.Context parameter, replace the
context.Background() call with this passed context, and then update all call
sites (including the one at lines 742-746 and the additional occurrences at
lines 914-916 and 930-931) to pass the poll or cleanup context through to the
helper function.
- Line 663: The cleanup operation in the defer statement for
cleanupProjectRequestTemplateTest needs to restore the cluster config before
deleting the template to prevent leaving the cluster in a broken state, and all
error returns from cleanup operations must be checked and handled rather than
ignored. Modify the cleanupProjectRequestTemplateTest function to first restore
the cluster configuration (using the restoreProjectSpec parameter) before
deleting the referenced template, and ensure that any errors returned from
restore operations or template deletion are properly checked and handled
according to Go error handling guidelines. Apply the same fix to all similar
cleanup defer statements mentioned in the extended comment range (lines
891-925).
- Around line 619-624: The deferred Execute() calls for project cleanup (the
delete project commands for firstProject and secondProject, and similar calls at
lines 805 and 872) are not checking for errors, which violates Go error handling
guidelines. Wrap each deferred project deletion in a pattern similar to the
new-project creation shown in the diff: capture the error from Execute() and
validate it with o.Expect(err).NotTo(o.HaveOccurred()) to ensure cleanup
failures are not silently ignored. Apply this error checking wrapper
consistently to all deferred project deletion calls throughout the file.
- Around line 380-385: The error returned from GetScopedClientForUser() for
twoThreeBobConfig is being ignored before the config is used to create
twoThreeBobClient with NewForConfigOrDie(). Add an error check immediately after
the GetScopedClientForUser() call (similar to how adjacent scoped client calls
handle errors) to verify err is nil before proceeding to construct
twoThreeBobClient. If an error occurs, handle it appropriately by returning or
logging the error rather than allowing a potentially invalid config to be passed
to NewForConfigOrDie().
- Around line 875-877: Replace the oc status command call and substring matching
in the assertion with direct resource queries. Instead of running oc status and
checking for the substring "no services, deployment", query each resource type
deterministically by using separate oc get commands for configmaps, secrets,
pods, services, and deployments with the projectName filter, then verify that
each resource query returns no results. This approach is more reliable than
parsing the human-readable prose output of oc status.
- Around line 995-1002: The defer block in the GetScopedClientForUser function
deletes the OAuthAccessToken prematurely before callers can use it with the
returned scopedConfig for watches and list operations. Remove the entire defer
block that calls Delete on sha256TokenStr so the token persists after the
function returns and is available for callers to use. Additionally, if token
cleanup is needed, it should be handled by calling code at an appropriate time,
not immediately deferred here.
- Around line 821-831: The polling logic inside the PollUntilContextTimeout call
with the regexp.MatchString check is insufficient because it only filters out
early lifecycle states (ContainerCreating, Init, Pending) but does not verify
that pods are actually in a Running or Ready state. This allows the poll to
succeed even when pods are in failure states like CrashLoopBackOff, Error, or
Completed. Replace the negative matching logic (checking if pods are NOT in
certain states) with positive matching logic that explicitly checks the pod
output contains the "Running" state, ensuring the poll only succeeds when pods
are actually healthy and running.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 552040f8-5613-45f8-8f7c-f5a447a15b1f
📒 Files selected for processing (3)
test/extended/project/project.gotest/extended/testdata/bindata.gotest/extended/testdata/project/project-request-limits-quota.yaml
✅ Files skipped from review due to trivial changes (2)
- test/extended/testdata/project/project-request-limits-quota.yaml
- test/extended/testdata/bindata.go
|
Scheduling required tests: |
|
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: bc9b7f9
New tests seen in this PR at sha: bc9b7f9
|
YamunadeviShanmugam
force-pushed
the
OTE_migrate_apiserver
branch
from
bc9b7f9 to
b912f72
Compare
|
Scheduling required tests: |
|
Job Failure Risk Analysis for sha: b912f72
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: b912f72
New tests seen in this PR at sha: b912f72
|
YamunadeviShanmugam
force-pushed
the
OTE_migrate_apiserver
branch
from
b912f72 to
6cb4954
Compare
openshift-ci
Bot
added
the
ready-for-human-review
|
/lgtm |
|
Scheduling required tests: |
Migrate four project API tests to OTE
YamunadeviShanmugam
force-pushed
the
OTE_migrate_apiserver
branch
from
6cb4954 to
a932f0a
Compare
|
/lgtm |
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
|
/retest |
|
Scheduling required tests: |
|
/unhold |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
/retest |
|
@YamunadeviShanmugam: all tests passed! 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 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: gangwgr, kaleemsiddiqu, YamunadeviShanmugam 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 |
|
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: a932f0a
New tests seen in this PR at sha: a932f0a
|
|
/verified by CI |
openshift-ci-robot
added
the
verified
|
@YamunadeviShanmugam: This PR has been marked as verified by 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. |
5 participants
Summary
Migrates project API test cases to the OpenShift Tests Extension (OTE) framework, following OTE integration guidelines and origin test standards.
Tests Added
1. Invalid Node Selector Validation
[OTP] should reject project creation when an invalid node selector is given2. Node Selector Display
[OTP] should allow a user to get the node selector from a projectoc describe projectcorrectly displays node selectors for projects with and without selectors3. Project Request Template Configuration
[OTP] should apply a customized project request template with ResourceQuota and LimitRange4. Cascading Project Deletion
[OTP] should delete all resources when the project is deletedSummary by CodeRabbit
Summary
<none>when unset, configured values when set).ResourceQuota/LimitRangeonly to projects created after readiness.