-
Notifications
You must be signed in to change notification settings - Fork 50
OCPCLOUD-3326: Update to use new manifests-gen #584
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Skipping CI for Draft Pull Request. |
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)
No actionable comments were generated in the recent review. 🎉 Comment |
|
@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. |
ce4b500 to
799b643
Compare
1743766 to
cb7778f
Compare
cb7778f to
732cd52
Compare
732cd52 to
e799452
Compare
e799452 to
db9ad01
Compare
|
/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 |
db9ad01 to
edeb50c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 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).
edeb50c to
1037ebf
Compare
|
/lgtm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| cd tools && $(MANIFESTS_GEN) --manifests-path "../capi-operator-manifests" --kustomize-dir="../../openshift" \ | ||
| --name cluster-api-provider-aws \ | ||
| --install-order 20\ | ||
| --attribute type=infrastructure \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix the line continuation after --install-order to avoid flag concatenation.
Without a space before the backslash, the shell concatenates 20 with --attribute, so manifests-gen receives --install-order 20--attribute and the --attribute flag is lost, likely breaking the command.
Proposed fix
- --install-order 20\
+ --install-order 20 \📝 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.
| cd tools && $(MANIFESTS_GEN) --manifests-path "../capi-operator-manifests" --kustomize-dir="../../openshift" \ | |
| --name cluster-api-provider-aws \ | |
| --install-order 20\ | |
| --attribute type=infrastructure \ | |
| cd tools && $(MANIFESTS_GEN) --manifests-path "../capi-operator-manifests" --kustomize-dir="../../openshift" \ | |
| --name cluster-api-provider-aws \ | |
| --install-order 20 \ | |
| --attribute type=infrastructure \ |
🤖 Prompt for AI Agents
In `@openshift/Makefile` around lines 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 |
|
New changes are detected. LGTM label has been removed. |
|
@mdbooth: 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. |
No description provided.