OCPCLOUD-3513: Verify manifests in e2e#615
Conversation
|
@hongkailiu: This pull request references OCPCLOUD-3513 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. |
WalkthroughThis PR enhances the E2E test pipeline by adding a manifest verification gate. A new script validates that the cluster manifests are accessible and that a specific set of CRDs do not exist, then the E2E test orchestration is updated to invoke this verification before running the main test process. ChangesE2E Manifest Verification
Sequence DiagramsequenceDiagram
participant e2e-tests.sh
participant e2e-verify-manifests.sh
participant Cluster
e2e-tests.sh->>e2e-verify-manifests.sh: invoke
e2e-verify-manifests.sh->>Cluster: oc get -f (fetch manifest)
Cluster-->>e2e-verify-manifests.sh: manifest YAML
e2e-verify-manifests.sh->>Cluster: oc get crd (verify CRDs absent)
Cluster-->>e2e-verify-manifests.sh: --ignore-not-found
alt any CRD found
e2e-verify-manifests.sh->>e2e-verify-manifests.sh: exit 1
else all CRDs absent
e2e-verify-manifests.sh-->>e2e-tests.sh: continue
e2e-tests.sh->>e2e-tests.sh: make e2e
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (14 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[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.
Actionable comments posted: 1
🤖 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 `@openshift/e2e-verify-manifests.sh`:
- Around line 10-15: The loop that currently uses "if oc get crd ${crd}; then"
can mask API/auth/network failures as CRD absence; replace that conditional with
an explicit run of oc get crd "${crd}" capturing both its exit code and stderr
(e.g., store output="$(oc get crd "${crd}" 2>&1)"; status=$? ), then: if
status==0 -> treat as found and fail (same error/exit), else if output contains
"NotFound" or "not found" -> treat as absent and continue, else treat as an
API/auth/network error and exit non-zero with a clear error message. Apply this
change inside the for crd in "${arr[@]}"; do loop around the oc get crd ${crd}
invocation.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7c396b68-ca00-40ae-a8a5-9d625cf83f39
📒 Files selected for processing (2)
openshift/e2e-tests.shopenshift/e2e-verify-manifests.sh
8041741 to
cb3a1a4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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
`@docs/superpowers/specs/2026-06-11-exclude-awsmachinepools-openshift-design.md`:
- Around line 92-103: The patch for capa-manager-role fails to delete
awsmachinepools RBAC because the design combines three resources into one rule
without verbs so $patch: delete doesn't match the separately-generated
PolicyRules; update the patch for the ClusterRole named capa-manager-role to
include three distinct rules that exactly match the generated rules (one for
"awsmachinepools", one for "awsmachinepools/finalizers", one for
"awsmachinepools/status") including the same verbs arrays, each annotated with
$patch: delete so the kustomize patch matches and removes those PolicyRules.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3a7c3e13-b775-4061-a7f4-394d08149654
📒 Files selected for processing (4)
docs/superpowers/plans/2026-06-11-exclude-awsmachinepools-openshift.mddocs/superpowers/specs/2026-06-11-exclude-awsmachinepools-openshift-design.mdopenshift/e2e-tests.shopenshift/e2e-verify-manifests.sh
✅ Files skipped from review due to trivial changes (1)
- docs/superpowers/plans/2026-06-11-exclude-awsmachinepools-openshift.md
🚧 Files skipped from review as they are similar to previous changes (2)
- openshift/e2e-verify-manifests.sh
- openshift/e2e-tests.sh
cb3a1a4 to
a0cec9a
Compare
| oc get -f openshift/capi-operator-manifests/default/manifests.yaml | ||
|
|
||
| # TODO: Update the array below when https://redhat.atlassian.net/browse/OCPCLOUD-3537 is done | ||
| declare -a arr=("pod.not-exist.io" "svc.not-exist.io") |
There was a problem hiding this comment.
will you be able to get this in before the PR merges or is that for later? if later, can you create an issue for it if there isn't one
There was a problem hiding this comment.
This array with "pod.not-exist.io" and "svc.not-exist.io" will work after openshift/release#80470 gets in. They are the placeholder until we get the list in OCPCLOUD-3537.
The array is the list of CRDs that are not installed on the cluster and neither "pod.not-exist.io" nor "svc.not-exist.io" is. So we can merge this one as it is.
When https://redhat.atlassian.net/browse/OCPCLOUD-3537 is done, I am supposed to refresh the list with the real ones to filter out. I added an item into DoD to remind me of it.
|
Firstly, I don't think that I think a more robust method, which would also be simpler to review, would be to have a file containing a specific allowed list. Perhaps a simple list of CRD names, one per line. You'd also need one of these files for each profile. The verify step could parse CRDs out of the target profile's manifests and ensure that the resulting set is identical to the expected set. This also makes changes to the expected set very easy to review. Thoughts? |
|
/test e2e-aws-capi-techpreview |
|
@hongkailiu: 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. |
/hold
It requires openshift/release#80470 to get in first.
Summary by CodeRabbit