OCPCLOUD-3326: Update to use new manifests-gen#584
OCPCLOUD-3326: Update to use new manifests-gen#584mdbooth wants to merge 3 commits intoopenshift:mainfrom
Conversation
|
Skipping CI for Draft Pull Request. |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Review skipped — only excluded labels are configured. (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
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:
WalkthroughUpdates OpenShift CAPI operator packaging: changes Dockerfile manifest copy paths, replaces the Makefile's ocp-manifests invocation with a more granular manifests-gen call, adds kustomize and provider metadata under Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
mdbooth
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@mdbooth: This pull request references OCPCLOUD-3326 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.22.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. |
mdbooth
force-pushed
the
manifests-gen
branch
from
ce4b500 to
799b643
Compare
theobarberbany
force-pushed
the
manifests-gen
branch
from
1743766 to
cb7778f
Compare
openshift-merge-robot
added
the
needs-rebase
theobarberbany
force-pushed
the
manifests-gen
branch
from
cb7778f to
732cd52
Compare
openshift-merge-robot
removed
the
needs-rebase
theobarberbany
force-pushed
the
manifests-gen
branch
from
732cd52 to
e799452
Compare
theobarberbany
force-pushed
the
manifests-gen
branch
from
e799452 to
db9ad01
Compare
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: theobarberbany 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 |
openshift-ci
bot
added
the
approved
theobarberbany
force-pushed
the
manifests-gen
branch
from
db9ad01 to
edeb50c
Compare
![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
🤖 Fix all issues with AI agents
In `@openshift/kustomization.yaml`:
- Around line 21-23: The JSON Patch path for the remove op is missing the
closing brace and thus doesn't match the annotation key; update the patch entry
whose path currently reads
/metadata/annotations/${AWS_CONTROLLER_IAM_ROLE~1#arn~1eks.amazonaws.com~1role-arn
to include the trailing } so it exactly matches the annotation key
`${AWS_CONTROLLER_IAM_ROLE/#arn/eks.amazonaws.com/role-arn}` (keeping the ~1
separators as used in the patch encoding).
| patch: |- | ||
| - op: remove | ||
| path: /metadata/annotations/${AWS_CONTROLLER_IAM_ROLE~1#arn~1eks.amazonaws.com~1role-arn |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's read the openshift/kustomization.yaml file to see the exact content
cat -n openshift/kustomization.yamlRepository: openshift/cluster-api-provider-aws
Length of output: 841
🏁 Script executed:
# Search for AWS_CONTROLLER_IAM_ROLE references in the codebase
rg -n "AWS_CONTROLLER_IAM_ROLE" --type yaml -C 3Repository: openshift/cluster-api-provider-aws
Length of output: 1707
🏁 Script executed:
# Look for ServiceAccount definitions and annotation patterns
rg -n "metadata/annotations" --type yaml -C 2Repository: openshift/cluster-api-provider-aws
Length of output: 625
JSON Patch path is truncated - missing closing }.
The annotation key in config/rbac/serviceaccount.yaml is ${AWS_CONTROLLER_IAM_ROLE/#arn/eks.amazonaws.com/role-arn} (ends with }), but the patch path on line 23 omits the closing brace. This causes the patch to fail silently since it won't match any existing annotation.
Correct the path:
Diff
patch: |-
- op: remove
- path: /metadata/annotations/${AWS_CONTROLLER_IAM_ROLE~1#arn~1eks.amazonaws.com~1role-arn
+ path: /metadata/annotations/${AWS_CONTROLLER_IAM_ROLE~1#arn~1eks.amazonaws.com~1role-arn}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| patch: |- | |
| - op: remove | |
| path: /metadata/annotations/${AWS_CONTROLLER_IAM_ROLE~1#arn~1eks.amazonaws.com~1role-arn | |
| patch: |- | |
| - op: remove | |
| path: /metadata/annotations/${AWS_CONTROLLER_IAM_ROLE~1#arn~1eks.amazonaws.com~1role-arn} |
🤖 Prompt for AI Agents
In `@openshift/kustomization.yaml` around lines 21 - 23, The JSON Patch path for
the remove op is missing the closing brace and thus doesn't match the annotation
key; update the patch entry whose path currently reads
/metadata/annotations/${AWS_CONTROLLER_IAM_ROLE~1#arn~1eks.amazonaws.com~1role-arn
to include the trailing } so it exactly matches the annotation key
`${AWS_CONTROLLER_IAM_ROLE/#arn/eks.amazonaws.com/role-arn}` (keeping the ~1
separators as used in the patch encoding).
There was a problem hiding this comment.
Added comment - this is intentional and also a disgusting hack.
theobarberbany
force-pushed
the
manifests-gen
branch
from
edeb50c to
1037ebf
Compare
|
/lgtm |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@openshift/Makefile`:
- Around line 26-29: The shell command in the Makefile that invokes
$(MANIFESTS_GEN) is concatenating the `--install-order` value and the next flag
because the backslash is immediately after `20`; update the line with
`--install-order 20\` so there is a space before the backslash (e.g.,
`--install-order 20 \`) to ensure `--attribute` (and subsequent flags) are
passed as separate arguments to the `MANIFESTS_GEN` invocation.
|
/retest |
1 similar comment
|
/retest |
theobarberbany
force-pushed
the
manifests-gen
branch
2 times, most recently
from
d67f480 to
bfd5d73
Compare
Adds openshift/kustomization.yaml to generate OpenShift provider
manifests from the upstream kustomize base (config/default).
The kustomization includes a JSON Patch to remove an unsubstituted
envsubst annotation from the upstream ServiceAccount. This annotation
uses drone/envsubst prefix substitution syntax intended for clusterctl:
${AWS_CONTROLLER_IAM_ROLE/#arn/eks.amazonaws.com/role-arn: arn}
When AWS_CONTROLLER_IAM_ROLE is set (e.g. arn:aws:iam::123:role/foo),
envsubst expands the whole expression into:
eks.amazonaws.com/role-arn: arn:aws:iam::123:role/foo
OpenShift does not use clusterctl, so envsubst never runs and this
line remains as a literal in the YAML. The YAML parser splits it at
the first ": " (colon-space), producing:
Key: ${AWS_CONTROLLER_IAM_ROLE/#arn/eks.amazonaws.com/role-arn
Value: arn}
Note the annotation key has an opening { but NO closing } — the }
is part of the value. The JSON Patch path therefore must also omit
the closing }, with / encoded as ~1 per RFC 6901:
/metadata/annotations/${AWS_CONTROLLER_IAM_ROLE~1#arn~1eks.amazonaws.com~1role-arn
The unmatched { is intentional — it is a regular character in JSON
Pointer paths and matches the YAML-parsed annotation key exactly.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Two issues prevent upstream unit tests from passing in OpenShift CI: 1. CRD path resolution fails with vendoring. The upstream repo does not vendor, so `go test` uses module mode and downloads full module sources (including non-Go assets like CRD YAMLs) to $GOPATH/pkg/mod. The OpenShift fork vendors dependencies, and Go 1.14+ defaults to -mod=vendor when a vendor/ directory is present. Vendor directories only contain Go source files, so the cluster-api CRD YAMLs at $GOPATH/pkg/mod/sigs.k8s.io/cluster-api@.../config/crd/bases are never downloaded, causing test helpers to panic. Fix: explicitly set GOFLAGS=-mod=mod in unit-tests.sh to force module mode, matching upstream behaviour. 2. TestGetManagerNamespace fails due to mounted service account. GetManagerNamespace() reads /var/run/secrets/.../namespace before falling back to the POD_NAMESPACE env var. Upstream Prow runs tests on an EKS cluster where service account tokens are not automounted, so the file does not exist and the env var path is exercised. OpenShift CI (ci-operator) creates test pods with service accounts mounted, so the file exists and returns the CI namespace (ci-op-*) regardless of the env var the test sets. Fix: change inClusterNamespacePath from const to var so the test can override it, ensuring the env var fallback path is exercised in any CI environment. Additionally aligns unit-tests.sh with upstream scripts/ci-test.sh: source hack/ensure-go.sh and use make test-verbose. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
theobarberbany
force-pushed
the
manifests-gen
branch
from
bfd5d73 to
8e691fb
Compare
|
/retest |
|
/test e2e-aws-capi-techpreview We expect regression to fail until we test that expects CVO to be deploying manifests. |
|
/lgtm |
|
/override ci/prow/e2e-aws-capi-techpreview |
|
@theobarberbany: Overrode contexts on behalf of theobarberbany: ci/prow/e2e-aws-capi-techpreview 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. |
|
@mdbooth: 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
No description provided.