Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@rikatz: This pull request references NE-2750 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 "5.0.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. |
|
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:
WalkthroughChangesThe PR updates OpenShift API dependencies and adds an end-to-end Gateway API management-mode suite. The suite covers mode transitions, resource preservation, takeover blocking, recovery, routing, conditions, ClusterOperator status, and metrics. Gateway API management mode
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Change: Other Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant TestSuite
participant Ingress
participant GatewayAPI
participant ClusterOperator
participant LoadBalancer
TestSuite->>Ingress: Set Managed or Unmanaged mode
Ingress->>GatewayAPI: Reconcile CRDs, VAP, Gateway, and HTTPRoute
GatewayAPI->>ClusterOperator: Report management and compliance conditions
TestSuite->>LoadBalancer: Connect using the route hostname
LoadBalancer-->>TestSuite: Return HTTP response
Merge Risk: 🔵 Low · up to Cleanup defects can leave resources or annotations behind and make later Gateway API tests unreliable, but the impact is confined to test execution. Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 3 warnings)
✅ Passed checks (11 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 46.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 15 functions across 2 files. (1 skipped: 1 unsupported.) Full details: Test Structure And QualityExplanation The added suite has clear assertion-message violations. It contains 55 Resolution Add meaningful operation- and resource-specific messages to every message-less assertion in the added suite, and remove the duplicate assertion at line 71. Register CRD-annotation restoration before the mutation, restore the original annotation state in cleanup, check the update error, and wait for successful restoration. Make mock-CRD cleanup handle non-NotFound delete errors and wait for deletion. Register management-mode cleanup before any transition that can leave the singleton Ingress in Unmanaged mode, so failures during setup or state capture cannot leave the cluster in that mode. Full details: Ipv6 And Disconnected Network Test CompatibilityExplanation The new serial Ginkgo suite can fail on IPv6-only CI. In Resolution IPv6 and disconnected network compatibility notice: This test contains an IPv6 URL-construction assumption that can fail in IPv6-only environments. Run the additional serial CI job Full details: No-Sensitive-Data-In-LogsExplanation The pull request adds sensitive endpoint values to test logs. Resolution Remove ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@rikatz: This PR was included in a payload test run from openshift/cluster-ingress-operator#1547
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/baf0cf60-958c-11f1-8ef9-db390a0f6457-0 |
|
@rikatz: This PR was included in a payload test run from openshift/cluster-ingress-operator#1547
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/d88faa50-958c-11f1-966f-44422d7a35b5-0 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
test/extended/router/gatewayapi_management_mode.go (2)
224-224: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
platformAwareTimeoutfor consistency.Every other transition wait in this file wraps the timeout with
platformAwareTimeout. This call hardcodes5*time.Minute. On slow platforms the surrounding calls scale, but this one does not.♻️ Proposed change
- err = waitForManagementModeTransition(ctx, oc, operatorv1alpha1.GatewayAPIManagementModeManaged, 5*time.Minute) + err = waitForManagementModeTransition(ctx, oc, operatorv1alpha1.GatewayAPIManagementModeManaged, platformAwareTimeout(oc, 5*time.Minute))🤖 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/router/gatewayapi_management_mode.go` at line 224, Update the waitForManagementModeTransition call for GatewayAPIManagementModeManaged to pass platformAwareTimeout(5*time.Minute) instead of the hardcoded 5*time.Minute, matching the other transition waits in the file.
839-841: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
ptr.Tofor the boolean pointer.
k8s.io/utils/ptrprovidesptr.To(true)and is already used by extended tests. This removes the single-useboolPtrhelper.🤖 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/router/gatewayapi_management_mode.go` around lines 839 - 841, Replace the single-use boolPtr helper with k8s.io/utils/ptr.To at its call sites, using ptr.To for boolean pointers and removing boolPtr once unused.test/extended/router/gatewayapi_management_mode_upgrade.go (2)
293-306: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDetach cleanup from the canceled test context.
Teardown receives
ctxfrom the upgrade framework. If the spec context is canceled after a failure, every client call in Teardown fails immediately and the Gateway, HTTPRoute, and GatewayClass leak into the cluster. Detach cancellation and apply an explicit timeout.♻️ Proposed change
func (t *GatewayAPIManagementModeUpgradeTest) Teardown(ctx context.Context, f *e2e.Framework) { if t.oc == nil || t.gatewayName == "" { e2e.Logf("Skipping cleanup because setup did not initialize resources") return } + + ctx, cancel := context.WithTimeout(context.WithoutCancel(ctx), 10*time.Minute) + defer cancel()Based on learnings, in openshift/origin test helpers avoid
context.Background()for deferred cleanup; detach cancellation withcontext.WithoutCancel(ctx)to preserve context values, then bound it withcontext.WithTimeout.🤖 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/router/gatewayapi_management_mode_upgrade.go` around lines 293 - 306, Update GatewayAPIManagementModeUpgradeTest.Teardown to derive a cleanup context with context.WithoutCancel(ctx), then wrap it with an explicit timeout and defer its cancellation. Use this bounded, cancellation-independent context for setManagementMode and waitForManagementModeTransition so cleanup still runs after the test context is canceled.Source: Learnings
294-297: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winClean up the GatewayClass when Gateway creation does not complete.
The guard returns early when
t.gatewayNameis empty. Setup setst.gatewayClassNameat line 109 and creates the GatewayClass at line 111, before it setst.gatewayNameat line 124. If Setup fails between those points, the GatewayClass stays in the cluster. Gate each delete on its own recorded name.♻️ Proposed change
- if t.oc == nil || t.gatewayName == "" { + if t.oc == nil || (t.gatewayClassName == "" && t.gatewayName == "") { e2e.Logf("Skipping cleanup because setup did not initialize resources") return }Then guard the individual delete steps with
if t.routeName != "",if t.gatewayName != "", andif t.gatewayClassName != "".🤖 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/router/gatewayapi_management_mode_upgrade.go` around lines 294 - 297, Update the cleanup method’s initial guard so it only skips when the test client is unavailable, then gate each resource deletion independently using t.routeName, t.gatewayName, and t.gatewayClassName. This must delete the GatewayClass even when Gateway creation failed after its name was recorded, while preserving skips for empty resource names.
🤖 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 `@go.mod`:
- Around line 68-71: Run go mod tidy followed by go mod vendor to refresh
dependency metadata and vendored sources for the OpenShift modules in go.mod,
removing obsolete go.sum checksums for prior API and client-go versions while
retaining the versions that provide the required symbols.
In `@test/extended/router/gatewayapi_management_mode_upgrade.go`:
- Around line 120-125: Update the custom-domain setup near
getDefaultIngressClusterDomainName and the customDomain assignment to verify
that replacing "apps." actually changes defaultIngressDomain before using it;
fail the test clearly when the expected segment is absent, while preserving the
existing gateway hostname construction.
- Around line 89-106: Update Teardown to restore the recorded initial mode from
t.startMode rather than the post-upgrade current mode, preserving the original
cluster state. Keep Managed mode during any resource-deletion steps that require
it, then transition to t.startMode as the final cleanup action and wait for that
transition to complete.
- Around line 47-73: Update GatewayAPIManagementModeUpgradeTest.Skip so this
scenario is excluded from real upgrade runs on TechPreviewNoUpgrade clusters; do
not allow those clusters to proceed into Setup. Move the scenario to a
non-upgrade suite or gate it on a feature configuration that supports upgrades,
while preserving the existing skip checks for other environments.
In `@test/extended/router/gatewayapi_management_mode.go`:
- Around line 509-517: The VAP binding cleanup in the DeferCleanup callback must
clear metadata that cannot be reused on create, including UID and
CreationTimestamp alongside ResourceVersion. Handle Get errors other than
NotFound by reporting or failing cleanup instead of silently skipping
restoration, while preserving the existing recreation path when the binding is
absent.
- Around line 843-855: Update platformAwareTimeout to return baseTimeout when
infra.Status.PlatformStatus is nil before dereferencing it. Rename the
infrastructure and type variables to reflect their values, compare the platform
against configv1.PowerVSPlatformType instead of "IBMPowerVS", and remove
"IBMZPlatform" as a platform-type check; if IBM Z requires the multiplier,
determine it from node architecture instead.
---
Nitpick comments:
In `@test/extended/router/gatewayapi_management_mode_upgrade.go`:
- Around line 293-306: Update GatewayAPIManagementModeUpgradeTest.Teardown to
derive a cleanup context with context.WithoutCancel(ctx), then wrap it with an
explicit timeout and defer its cancellation. Use this bounded,
cancellation-independent context for setManagementMode and
waitForManagementModeTransition so cleanup still runs after the test context is
canceled.
- Around line 294-297: Update the cleanup method’s initial guard so it only
skips when the test client is unavailable, then gate each resource deletion
independently using t.routeName, t.gatewayName, and t.gatewayClassName. This
must delete the GatewayClass even when Gateway creation failed after its name
was recorded, while preserving skips for empty resource names.
In `@test/extended/router/gatewayapi_management_mode.go`:
- Line 224: Update the waitForManagementModeTransition call for
GatewayAPIManagementModeManaged to pass platformAwareTimeout(5*time.Minute)
instead of the hardcoded 5*time.Minute, matching the other transition waits in
the file.
- Around line 839-841: Replace the single-use boolPtr helper with
k8s.io/utils/ptr.To at its call sites, using ptr.To for boolean pointers and
removing boolPtr once unused.
🪄 Autofix
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: Pro Plus
Run ID: f8ee33e6-558f-4d06-a5bf-be02434d242e
⛔ Files ignored due to path filters (60)
go.sumis excluded by!**/*.sumvendor/github.com/openshift/api/config/v1/types_authentication.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_infrastructure.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_ingress.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_kmsencryption.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1alpha1/types_cluster_monitoring.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1alpha1/zz_generated.model_name.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/envtest-releases.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/features.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/features/features.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/features/legacyfeaturegates.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/types_kmsencryption.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1/zz_generated.model_name.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1alpha1/register.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/types_ingress.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.model_name.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/route/v1/generated.protois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/route/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/route/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/awsplatformstatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/baremetalplatformstatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/gcpplatformstatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/vaultkmspluginconfig.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/vsphereplatformfailuredomainspec.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/vsphereplatformspec.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/nodeexportercollectorconfig.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/nodeexportercollectordevicemappermultipathconfig.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/nodeexportercollectornvmexpresssubsystemconfig.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/nodeexportercollectorzoneinfoconfig.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/remotewritespec.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/internal/internal.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/utils.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/machineconfiguration/applyconfigurations/machineconfiguration/v1/controllerconfigspec.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/internal/internal.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1/kmsencryptionstatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1/kmspluginhealthreport.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1/kmspreflightcheck.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1/kmspreflightresult.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1/nodestatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1alpha1/gatewayapiingressconfig.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1alpha1/ingress.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1alpha1/ingressspec.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1alpha1/ingressstatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/clientset/versioned/typed/operator/v1alpha1/generated_expansion.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/clientset/versioned/typed/operator/v1alpha1/ingress.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/clientset/versioned/typed/operator/v1alpha1/operator_client.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (4)
go.modtest/e2e/upgrade/upgrade.gotest/extended/router/gatewayapi_management_mode.gotest/extended/router/gatewayapi_management_mode_upgrade.go
1cac6c8 to
bf75d20
Compare
|
@rikatz: This PR was included in a payload test run from openshift/cluster-ingress-operator#1547 |
|
/test help |
|
@rikatz: This PR was included in a payload test run from openshift/cluster-ingress-operator#1547
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/983aa1e0-95b3-11f1-8279-e88e2a3dac51-0 |
|
@rikatz: This PR was included in a payload test run from openshift/cluster-ingress-operator#1547 |
|
@rikatz: This PR was included in a payload test run from openshift/cluster-ingress-operator#1547
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/ec5620d0-95b6-11f1-8eee-421ddecb8633-0 |
bf75d20 to
c643657
Compare
|
@rikatz: This pull request references NE-2750 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 "5.1.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. |
|
@rikatz: This PR was included in a payload test run from openshift/cluster-ingress-operator#1547
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/2ae116a0-9a7d-11f1-9f96-19ccdddaaff4-0 |
|
Scheduling required tests: Scheduling tests matching the |
|
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: c643657
New tests seen in this PR at sha: c643657
|
c643657 to
832fb49
Compare
|
@rikatz: This PR was included in a payload test run from openshift/cluster-ingress-operator#1547
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/3842f840-9b09-11f1-93ca-8ae1857b1f3b-0 |
|
Scheduling required tests: Scheduling tests matching the |
|
@rikatz: This PR was included in a payload test run from openshift/cluster-ingress-operator#1547
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/3024fbc0-9b38-11f1-8309-2a9ce90cc78a-0 |
|
Job Failure Risk Analysis for sha: 832fb49
Risk analysis has seen new tests most likely introduced by this PR. New tests seen in this PR at sha: 832fb49
|
832fb49 to
55de148
Compare
|
@rikatz: This PR was included in a payload test run from openshift/cluster-ingress-operator#1547
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/10c590d0-9b53-11f1-8bd9-524d1609fbe0-0 |
|
/pipeline required |
|
Scheduling tests matching the |
|
@coderabbitai I am still waiting for your approval on this PR to mark it as ready for humans, I have addressed all of your comments |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="openshift/origin"
pr="31503"
echo "== Pull request state =="
gh pr view "$pr" --repo "$repo" \
--json headRefOid,mergeable,reviewDecision,statusCheckRollup,title,body \
--jq '{headRefOid,mergeable,reviewDecision,title,body,statusChecks:[.statusCheckRollup[]? | {name,status,conclusion}]}'
echo "== Changed files =="
gh pr view "$pr" --repo "$repo" --json files \
--jq '.files[] | {path,additions,deletions}'
echo "== Unresolved CodeRabbit review threads =="
gh api graphql \
-F owner="openshift" \
-F name="origin" \
-F number="$pr" \
-f query='
query($owner: String!, $name: String!, $number: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $number) {
reviewThreads(first: 100) {
nodes {
id
isResolved
path
line
originalLine
comments(first: 20) {
nodes { author { login } body createdAt }
}
}
}
}
}
}' \
--jq '.data.repository.pullRequest.reviewThreads.nodes[]
| select(.isResolved == false)
| select(any(.comments.nodes[]?; .author.login == "coderabbitai[bot]" or .author.login == "coderabbitai"))
| {id,path,line,originalLine,comments:[.comments.nodes[] | {author:.author.login,body,createdAt}]}'
echo "== Current diff validation =="
base="$(git merge-base HEAD origin/main 2>/dev/null || git merge-base HEAD main)"
git diff --check "$base"...HEAD
git diff --stat "$base"...HEAD
echo "== Gateway API test maps =="
ast-grep outline test/extended/router/gatewayapi_management_mode.go --items all --type function --view compact
ast-grep outline test/extended/router/gatewayapicontroller.go --items all --type function --match 'assertGatewayLoadbalancerReady|assertHttpRouteConnectionViaAddress' --view compact
echo "== Relevant current implementations =="
rg -n -C 10 --type go \
'should report correct metrics for management mode|ControlPlaneTopology|ExternalTopologyMode|NewE2EPrometheusRouterClient|func assertGatewayLoadbalancerReady|func assertHttpRouteConnectionViaAddress' \
test/extended/router/gatewayapi_management_mode.go \
test/extended/router/gatewayapicontroller.go
echo "== Upgrade claims in current PR body and changed source =="
gh pr view "$pr" --repo "$repo" --json body --jq -r '.body' | rg -ni -C 2 'upgrade' || true
rg -n -i -C 2 'upgrade|UpgradeTest' \
test/extended/router/gatewayapi_management_mode.go \
test/extended/router/gatewayapicontroller.go || trueLength of output: 16900 🏁 Script executed: #!/bin/bash
set -euo pipefail
base="$(git merge-base HEAD origin/main 2>/dev/null || git merge-base HEAD main)"
echo "== Source maps =="
ast-grep outline test/extended/router/gatewayapi_management_mode.go --items all --type function --view signatures
ast-grep outline test/extended/router/gatewayapicontroller.go --items all --type function --match 'assertGatewayLoadbalancerReady|assertHttpRouteConnectionViaAddress' --view signatures
echo "== Management-mode test diff =="
git diff --unified=20 "$base"...HEAD -- test/extended/router/gatewayapi_management_mode.go
echo "== Gateway controller helper diff =="
git diff --unified=20 "$base"...HEAD -- test/extended/router/gatewayapicontroller.go
echo "== Upgrade registration diff and context =="
git diff --unified=30 "$base"...HEAD -- test/extended/router/gatewayapi_upgrade.go
nl -ba test/extended/router/gatewayapi_upgrade.go | sed -n '1,220p'
echo "== Metrics topology guard =="
rg -n -C 18 --type go \
'should report correct metrics for management mode|ControlPlaneTopology|ExternalTopologyMode|NewE2EPrometheusRouterClient' \
test/extended/router/gatewayapi_management_mode.go
echo "== Load-balancer helper implementations and calls =="
rg -n -C 12 --type go \
'func assertGatewayLoadbalancerReady|func assertHttpRouteConnectionViaAddress|assertGatewayLoadbalancerReady\s*\(|assertHttpRouteConnectionViaAddress\s*\(' \
test/extended/router/gatewayapicontroller.go \
test/extended/router/gatewayapi_management_mode.go
echo "== Go formatting check for changed Go files =="
gofmt -d \
test/extended/router/gatewayapi_management_mode.go \
test/extended/router/gatewayapi_upgrade.go \
test/extended/router/gatewayapicontroller.goLength of output: 88436
I found no remaining CodeRabbit concerns. The PR has no unresolved CodeRabbit review threads. The diff passes whitespace and Go formatting checks. The External/HyperShift metrics guard is present. The PR description and release summary do not claim upgrade coverage. The current GitHub review decision still shows You are interacting with an AI system. |
|
@coderabbitai approve |
✅ Action performedComments resolved and changes approved. |
openshift-ci
Bot
added
the
ready-for-human-review
|
/lgtm |
|
/override-sticky ci/prow/e2e-aws-ovn-microshift-serial Automated triage: This failure appears unrelated to the PR changes. Job classification: Eligible long-running presubmit e2e/integration job: AWS OVN MicroShift serial conformance on Revision check: incoming/Prow run SHA Execution status: Tests executed. The test phase ran for 39m12s and produced
Completed supporting jobs: No completed supporting e2e jobs. Completed non-e2e signals: Fleet-wide failure rate: The exact failure is deterministic outside this PR: the MicroShift serial job failed on 17/17 PR runs in the reported Sep 11–15 window, and the corresponding periodic job failed 28/28 times. Regular OCP comparison jobs passed the same tests (10/10, 9/9, and 9/9 in the queried samples). This is a known MicroShift platform/test-surface defect, not an intermittent test flake. Overlap assessment: The PR adds Gateway API management-mode router tests and updates related Gateway API dependencies/vendor API. It does not change CSI, storage, VolumeGroupSnapshot APIs, or MicroShift conformance setup. The failing test surface has no direct or indirect overlap with the PR. Missing-coverage risk: Low for the failure being waived: the only blocking failures are the unrelated storage tests, while the run completed 88 other tests and the PR's changed surface is Gateway API/router. Residual risk remains for the still-pending e2e checks; those are not being treated as positive signal. Rationale: MicroShift does not provide the VolumeGroupSnapshotClass API required by these tests, yielding a repeatable API 404 across the fleet. The current run's artifacts and logs confirm the same failure on the live PR revision. If you disagree with this assessment, rerun the current job with AI-generated. Review for accuracy. |
|
@redhat-chai-bot: Overrode contexts on behalf of redhat-chai-bot: ci/prow/e2e-aws-ovn-microshift-serial These overrides will persist across retests on the current HEAD SHA. Pushing a new commit will clear them. Use 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 kubernetes-sigs/prow repository. |
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
| skip, reason, err := shouldSkipGatewayAPITests(oc, noOLM) | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
| o.Expect(err).NotTo(o.HaveOccurred()) |
There was a problem hiding this comment.
what is the reason for having double o.Expect(err).NotTo(o.HaveOccurred()) isnt it the same result twice?
There was a problem hiding this comment.
better be safe than sorry? jk, probably a mistake from my side, fixed
| mode = operatorv1alpha1.GatewayAPIManagementModeManaged | ||
| } | ||
| o.Expect(mode).To(o.Equal(operatorv1alpha1.GatewayAPIManagementModeManaged), | ||
| "Expected Ingress CR to have Managed mode by default") |
There was a problem hiding this comment.
should this area be polled? does the management mode section update instantaneously?
There was a problem hiding this comment.
this is right. This is part of the beginning of the test, we are not changing the management mode yet, so the idea is to get it and guarantee it is on Managed mode. No fix required here
|
|
||
| g.By("Attempting to switch to Managed mode (should be blocked)") | ||
| err = setManagementMode(ctx, oc, operatorv1alpha1.GatewayAPIManagementModeManaged) | ||
| o.Expect(err).NotTo(o.HaveOccurred()) |
There was a problem hiding this comment.
if the switch is blocked, does the err still return nil?
There was a problem hiding this comment.
no, this is right. Blocked here means "I update the management mode CR, and then I verify on the status that this is blocked".
I will remove the (should be blocked) as it is confusing on this step
| _, err = oc.AdminApiextensionsClient().ApiextensionsV1().CustomResourceDefinitions().Create(ctx, mockCRD, metav1.CreateOptions{}) | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
|
|
||
| g.DeferCleanup(func(ctx context.Context) { |
There was a problem hiding this comment.
do you mind adding a comment here for readability, saying this deletion is for if a test step fails. There are 2 deletions in this test case.
|
Job Failure Risk Analysis for sha: 69c36c9
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: 69c36c9
New tests seen in this PR at sha: 69c36c9
|
|
Job Failure Risk Analysis for sha: 69c36c9
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: 69c36c9
New tests seen in this PR at sha: 69c36c9
|
|
nice comments from @rhamini3 - update looks good /lgtm |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: gcs278, rikatz 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 |
|
/pipeline required |
|
Scheduling tests matching the |
|
@rikatz: This PR was included in a payload test run from openshift/cluster-ingress-operator#1595
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/01d266b0-b206-11f1-8178-2c21bbf48400-0 |
|
/override-sticky ci/prow/e2e-gcp-csi Automated triage: This failure appears unrelated to the PR changes. Job classification: Eligible long-running presubmit e2e/integration job. Revision check: run Execution status: Tests executed. JUnit reports 313 tests, 1 failure, 100 passed, 0 flaky, and 206 skipped. The exact failing test was Completed supporting jobs: Fleet-wide failure rate: The failed presubmit job had 3/3 successful runs outside this run in the last 14 days (100%). The exact test has 19/19 passes with 0 flakes in the current 7-day Sippy window; the test is GCP-only, so the GCP-specific rate is also 19/19 (100%). The 5.1 14-day raw history is 27 pass / 1 fail / 0 flake (96.4%); the single earlier failure was attributed to pod startup, and subsequent runs passed. Open regressions: The 5.1/main Component Readiness view was checked but unavailable due HTTP 403; no CR regression determination is possible from that view. Sippy reports 0 open bugs for the test. Linked bugs: None. No Overlap assessment: The PR changes Gateway API router test code in Missing-coverage risk: Low. The job executed the CSI suite and 100 other tests passed; the failed test's current fleet history is 19/19 passing with zero flakes, and the failure was caused by loss of API-server connectivity. This override does not replace the value of a future clean CSI run. Prior bot activity on this SHA: Rationale: The job is clearly override-eligible, tests ran, and the only failure is a transient API-server connectivity timeout. The exact CSI test is healthy in recent GCP history, the PR has no CSI/storage overlap, and no linked bug or open regression was found. If you disagree with this assessment, rerun the current job with AI-generated. Review for accuracy. |
|
@redhat-chai-bot: Overrode contexts on behalf of redhat-chai-bot: ci/prow/e2e-gcp-csi These overrides will persist across retests on the current HEAD SHA. Pushing a new commit will clear them. Use 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 kubernetes-sigs/prow repository. |
|
/override-sticky ci/prow/e2e-metal-ovn-two-node-fencing Automated triage: This failure appears unrelated to the PR changes. Job classification: Eligible long-running presubmit e2e/integration job: If you disagree with this assessment, rerun the current job with AI-generated. Review for accuracy. |
|
@redhat-chai-bot: Overrode contexts on behalf of redhat-chai-bot: ci/prow/e2e-metal-ovn-two-node-fencing These overrides will persist across retests on the current HEAD SHA. Pushing a new commit will clear them. Use 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 kubernetes-sigs/prow repository. |
|
@rikatz: The following test 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. |
5 participants
This change implements origin tests for Gateway API Management Mode feature.
They are intended to show the right working of this feature:
Summary by CodeRabbit
Tests
Chores