Double the ocm-agent memory limits#227
Merged
openshift-merge-bot[bot] merged 2 commits intoopenshift:masterfrom Mar 11, 2026
Merged
Conversation
WalkthroughAdds a new PKO migration tool and tests, updates boilerplate/CI/build images and E2E job templates, removes two OWNERS aliases, bumps several tool/image versions, and changes one OCMAgent memory constant from 64Mi to 128Mi. Changes
Sequence Diagram(s)sequenceDiagram
participant User as "Developer"
participant Script as "olm_pko_migration.py"
participant Git as "Git (remotes/branches)"
participant FS as "Filesystem (manifests)"
participant Gen as "Generators (Dockerfile, ClusterPackage, Tekton)"
participant Out as "Output Dir"
rect rgba(0,128,0,0.5)
User->>Script: run migrate CLI
end
Script->>Git: query remotes / default branch
Git-->>Script: remotes/branch info
Script->>FS: discover & load YAML manifests
FS-->>Script: manifest contents
Script->>Script: annotate/manipulate manifests (phases, templates)
Script->>Gen: generate Dockerfile / ClusterPackage / Tekton pipelines
Gen-->>Script: generated artifacts
Script->>Out: write converted manifests and templates
Out-->>User: files created (deploy_pko/, Dockerfile.pko, tekton/)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
openshift-ci
Bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/consts/ocmagenthandler/ocmagenthandler.go (1)
75-75: Add an exact assertion for the memory limit to prevent silent regressions.This change is fine, but current deployment tests only assert memory is
> 0, so they won’t catch a future accidental downgrade from128Mi.Suggested test tightening (in
pkg/ocmagenthandler/ocmagenthandler_deployment_test.go)-Expect(deployment.Spec.Template.Spec.Containers[0].Resources.Limits.Memory().Value()).To(BeNumerically(">", 0)) +expectedMemLimit := k8sresource.MustParse(oah.ResourceLimitsMemory) +Expect(deployment.Spec.Template.Spec.Containers[0].Resources.Limits.Memory().Cmp(expectedMemLimit)).To(Equal(0))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/consts/ocmagenthandler/ocmagenthandler.go` at line 75, The tests currently only assert the memory limit is > 0 which can miss regressions; update the deployment test in pkg/ocmagenthandler/ocmagenthandler_deployment_test.go to assert the memory limit equals the constant ResourceLimitsMemory (i.e., "128Mi") exactly instead of or in addition to the >0 check, locating the assertion that reads something like resource.Limits.Memory and replacing it with an equality/assertExact check against ResourceLimitsMemory to prevent silent downgrades.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/consts/ocmagenthandler/ocmagenthandler.go`:
- Line 75: The tests currently only assert the memory limit is > 0 which can
miss regressions; update the deployment test in
pkg/ocmagenthandler/ocmagenthandler_deployment_test.go to assert the memory
limit equals the constant ResourceLimitsMemory (i.e., "128Mi") exactly instead
of or in addition to the >0 check, locating the assertion that reads something
like resource.Limits.Memory and replacing it with an equality/assertExact check
against ResourceLimitsMemory to prevent silent downgrades.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9f015fc3-b188-4efc-97de-e674284c0507
📒 Files selected for processing (1)
pkg/consts/ocmagenthandler/ocmagenthandler.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #227 +/- ##
=======================================
Coverage 66.25% 66.25%
=======================================
Files 23 23
Lines 1544 1544
=======================================
Hits 1023 1023
Misses 445 445
Partials 76 76
🚀 New features to boost your workflow:
|
Contributor
|
/test validate |
| // ResourceLimitsCPU and ResourceLimitsMemory defines the cpu and memory limits for OA deployment | ||
| ResourceLimitsCPU = "50m" | ||
| ResourceLimitsMemory = "64Mi" | ||
| ResourceLimitsMemory = "128Mi" |
Contributor
There was a problem hiding this comment.
Based on MEM's req and limit, the QoS: 327%, which is over request at limit. Should we need to increase the req size as well based on OA pods regular usage?
Contributor
|
@tkong-redhat: 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. |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
boilerplate/openshift/golang-osd-operator/olm_pko_migration.py (4)
695-698: Remove unnecessary f-string prefix.These strings have no placeholders, so the
fprefix is extraneous.♻️ Proposed fix
- print(f"Please review this file and ensure the deployment targets match your expectation") + print("Please review this file and ensure the deployment targets match your expectation") else: print("⚠ Using default clusterDeploymentSelector (hack/olm-registry/olm-artifacts-template.yaml not found)") - print(f"Please review this file and ensure the deployment targets match your expectation") + print("Please review this file and ensure the deployment targets match your expectation")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@boilerplate/openshift/golang-osd-operator/olm_pko_migration.py` around lines 695 - 698, Remove the unnecessary f-string prefixes on the two print calls that have no placeholders — change print(f"Please review this file and ensure the deployment targets match your expectation") and the identical print in the else-block to plain string prints (print("Please review this file and ensure the deployment targets match your expectation")) so the extraneous f prefix is removed; locate these statements in olm_pko_migration.py near the existing print about "⚠ Using default clusterDeploymentSelector" and update them accordingly.
256-259: Use exception chaining for better tracebacks.When re-raising exceptions with a different type, using
raise ... from errpreserves the original traceback, which aids debugging.♻️ Proposed fix
except subprocess.CalledProcessError: raise RuntimeError( "Not in a git repository. This script must be run from within a git repository." - ) + ) from None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@boilerplate/openshift/golang-osd-operator/olm_pko_migration.py` around lines 256 - 259, The except block catching subprocess.CalledProcessError should preserve the original traceback by using exception chaining: change the handler in the block that currently reads "except subprocess.CalledProcessError:" to capture the exception (e.g., "except subprocess.CalledProcessError as err:") and re-raise the RuntimeError using "raise RuntimeError(... ) from err" so the original CalledProcessError traceback is preserved; locate the handler that catches subprocess.CalledProcessError in this file and update it accordingly.
574-576: Use explicitNonetype annotation.Per PEP 484, use
str | Noneinstead of implicitOptional.♻️ Proposed fix
def write_manifest( - manifest: dict[str, Any], directory: str, filename: str = None, force: bool = False + manifest: dict[str, Any], directory: str, filename: str | None = None, force: bool = False ) -> None:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@boilerplate/openshift/golang-osd-operator/olm_pko_migration.py` around lines 574 - 576, The function signature for write_manifest uses an implicit Optional by assigning None to filename; update the type annotation to be explicit using the PEP 484 union operator: change the parameter from filename: str = None to filename: str | None = None (leave default and other params unchanged) so the function definition becomes def write_manifest(manifest: dict[str, Any], directory: str, filename: str | None = None, force: bool = False) -> None; adjust any static type hints elsewhere calling write_manifest if needed.
563-566: Unhandled kinds are appended without phase annotation.When a manifest kind is unrecognized, it's appended without PKO phase annotations. This could cause issues during PKO reconciliation if the manifest lacks phase information. Consider adding a default phase or skipping unhandled kinds.
♻️ Option: Apply default phase to unhandled kinds
else: print(f"Unhandled type: {kind}") - annotated.append(manifest) + # Apply deploy phase as default for unhandled kinds + annotated.append(annotate(manifest, PHASE_DEPLOY))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@boilerplate/openshift/golang-osd-operator/olm_pko_migration.py` around lines 563 - 566, When encountering an unrecognized manifest kind in the else branch (where you currently print "Unhandled type: {kind}" and call annotated.append(manifest)), ensure the manifest contains a PKO phase annotation before appending; mutate manifest by ensuring metadata and annotations exist and set a default PKO phase (e.g. annotations['pko.openshift.io/phase'] = '0' or another agreed default) then append to annotated, or alternatively skip appending by returning/continuing if you prefer to ignore unhandled kinds—update the else branch that references kind, manifest, and annotated accordingly.boilerplate/openshift/golang-osd-operator/test_olm_pko_migration.py (2)
333-345: Consider using a more precise assertion.The test provides 3 manifests (2 valid, 1 invalid), so the expected result count is exactly 2. Using
assertEqualwould make the test more precise and catch regressions.♻️ Proposed improvement
# Should not raise an exception result = migration.annotate_manifests(manifests) # Should process the valid ones - self.assertGreaterEqual(len(result), 2) + self.assertEqual(len(result), 2)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@boilerplate/openshift/golang-osd-operator/test_olm_pko_migration.py` around lines 333 - 345, The test test_annotate_manifests_skips_invalid_yaml currently uses assertGreaterEqual(len(result), 2) which is imprecise; update the assertion to assertEqual(len(result), 2) so it verifies that annotate_manifests returns exactly the two valid manifests (refer to the annotate_manifests function and the test method name to locate the change).
419-424: Unnecessary mock decorator.
get_pko_manifest()takesoperator_nameas a direct parameter, so the@patch('olm_pko_migration.get_operator_name')mock is unused and can be removed.♻️ Proposed fix
- `@patch`('olm_pko_migration.get_operator_name') - def test_get_pko_manifest_structure(self, mock_get_name): + def test_get_pko_manifest_structure(self): """Test that PKO PackageManifest has correct structure.""" - mock_get_name.return_value = 'test-operator' - manifest = migration.get_pko_manifest('test-operator')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@boilerplate/openshift/golang-osd-operator/test_olm_pko_migration.py` around lines 419 - 424, Remove the unused patch on get_operator_name in the test_get_pko_manifest_structure test: delete the `@patch`('olm_pko_migration.get_operator_name') decorator, remove the mock parameter mock_get_name from the test signature, and delete the mock_get_name.return_value assignment since get_pko_manifest(operator_name) is called directly with 'test-operator' and does not use get_operator_name.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@boilerplate/openshift/golang-osd-e2e/e2e-template.yml`:
- Around line 7-26: The parameters block is missing a declaration for
SLACK_CHANNEL while the template still references ${SLACK_CHANNEL} in the
slackChannel field; add a parameter entry named SLACK_CHANNEL (e.g., "name:
SLACK_CHANNEL" and set appropriate required/value like other params) to
boilerplate/openshift/golang-osd-e2e/e2e-template.yml (and mirror the same
addition in test/e2e/e2e-template.yml) so the interpolation resolves at runtime.
In `@boilerplate/openshift/golang-osd-operator/olm_pko_migration.py`:
- Around line 441-452: The OpenAPI schema places "type": "object" inside
"properties" instead of alongside it; modify the dict under "config" ->
"openAPIV3Schema" so that "properties" (which includes "image") remains as-is
and the "type": "object" entry is moved out to be a sibling of "properties"
(i.e., openAPIV3Schema should contain both "type": "object" and "properties": {
"image": { ... } }), ensuring the generated PackageManifest has a valid OpenAPI
V3 schema.
In `@boilerplate/openshift/golang-osd-operator/standard.mk`:
- Around line 432-444: The two Make targets pko-migrate-no-dockerfile and
pko-migrate-no-tekton currently run ${CONVENTION_DIR}/olm_pko_migration.py
unguarded; add the same script-existence guard used by the pko-migrate target so
they first check that ${CONVENTION_DIR}/olm_pko_migration.py exists and print a
clear error/exit if missing, then run the python invocation; update the targets
pko-migrate-no-dockerfile and pko-migrate-no-tekton to mirror the guard logic
and messaging used by pko-migrate.
- Around line 274-281: The make recipe hardcodes linux/amd64 when invoking
$(SETUP_ENVTEST) which breaks non-linux/amd64 hosts; update the invocation to
use the existing Make vars $(GOOS) and $(GOARCH) instead of literal "linux" and
"amd64" so ASSETS_PATH is obtained for the current platform (affecting the code
around ASSETS_PATH assignment and the $(SETUP_ENVTEST) call that uses
$(ENVTEST_K8S_VERSION)); keep the rest of the logic (error handling, echo, and
exporting KUBEBUILDER_ASSETS to the ${GOENV} go test invocation) unchanged.
---
Nitpick comments:
In `@boilerplate/openshift/golang-osd-operator/olm_pko_migration.py`:
- Around line 695-698: Remove the unnecessary f-string prefixes on the two print
calls that have no placeholders — change print(f"Please review this file and
ensure the deployment targets match your expectation") and the identical print
in the else-block to plain string prints (print("Please review this file and
ensure the deployment targets match your expectation")) so the extraneous f
prefix is removed; locate these statements in olm_pko_migration.py near the
existing print about "⚠ Using default clusterDeploymentSelector" and update them
accordingly.
- Around line 256-259: The except block catching subprocess.CalledProcessError
should preserve the original traceback by using exception chaining: change the
handler in the block that currently reads "except
subprocess.CalledProcessError:" to capture the exception (e.g., "except
subprocess.CalledProcessError as err:") and re-raise the RuntimeError using
"raise RuntimeError(... ) from err" so the original CalledProcessError traceback
is preserved; locate the handler that catches subprocess.CalledProcessError in
this file and update it accordingly.
- Around line 574-576: The function signature for write_manifest uses an
implicit Optional by assigning None to filename; update the type annotation to
be explicit using the PEP 484 union operator: change the parameter from
filename: str = None to filename: str | None = None (leave default and other
params unchanged) so the function definition becomes def
write_manifest(manifest: dict[str, Any], directory: str, filename: str | None =
None, force: bool = False) -> None; adjust any static type hints elsewhere
calling write_manifest if needed.
- Around line 563-566: When encountering an unrecognized manifest kind in the
else branch (where you currently print "Unhandled type: {kind}" and call
annotated.append(manifest)), ensure the manifest contains a PKO phase annotation
before appending; mutate manifest by ensuring metadata and annotations exist and
set a default PKO phase (e.g. annotations['pko.openshift.io/phase'] = '0' or
another agreed default) then append to annotated, or alternatively skip
appending by returning/continuing if you prefer to ignore unhandled kinds—update
the else branch that references kind, manifest, and annotated accordingly.
In `@boilerplate/openshift/golang-osd-operator/test_olm_pko_migration.py`:
- Around line 333-345: The test test_annotate_manifests_skips_invalid_yaml
currently uses assertGreaterEqual(len(result), 2) which is imprecise; update the
assertion to assertEqual(len(result), 2) so it verifies that annotate_manifests
returns exactly the two valid manifests (refer to the annotate_manifests
function and the test method name to locate the change).
- Around line 419-424: Remove the unused patch on get_operator_name in the
test_get_pko_manifest_structure test: delete the
`@patch`('olm_pko_migration.get_operator_name') decorator, remove the mock
parameter mock_get_name from the test signature, and delete the
mock_get_name.return_value assignment since get_pko_manifest(operator_name) is
called directly with 'test-operator' and does not use get_operator_name.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 639b14ea-911c-46da-bbf2-d8ea47dfe115
📒 Files selected for processing (18)
.ci-operator.yamlOWNERS_ALIASESboilerplate/_data/backing-image-tagboilerplate/_data/last-boilerplate-commitboilerplate/openshift/golang-osd-e2e/e2e-template.ymlboilerplate/openshift/golang-osd-e2e/updateboilerplate/openshift/golang-osd-operator/Dockerfile.olm-registryboilerplate/openshift/golang-osd-operator/OWNERS_ALIASESboilerplate/openshift/golang-osd-operator/TEST_README.mdboilerplate/openshift/golang-osd-operator/ensure.shboilerplate/openshift/golang-osd-operator/olm_pko_migration.pyboilerplate/openshift/golang-osd-operator/standard.mkboilerplate/openshift/golang-osd-operator/test-requirements.txtboilerplate/openshift/golang-osd-operator/test_olm_pko_migration.pybuild/Dockerfilebuild/Dockerfile.olm-registrytest/e2e/Dockerfiletest/e2e/e2e-template.yml
💤 Files with no reviewable changes (2)
- OWNERS_ALIASES
- boilerplate/openshift/golang-osd-operator/OWNERS_ALIASES
✅ Files skipped from review due to trivial changes (5)
- build/Dockerfile
- boilerplate/openshift/golang-osd-operator/TEST_README.md
- boilerplate/_data/last-boilerplate-commit
- boilerplate/openshift/golang-osd-operator/test-requirements.txt
- boilerplate/_data/backing-image-tag
Comment on lines
+7
to
+26
| - name: OSDE2E_CONFIGS | ||
| required: true | ||
| - name: TEST_IMAGE | ||
| required: true | ||
| - name: OCM_CCS | ||
| required: false | ||
| - name: CLOUD_PROVIDER_REGION | ||
| required: false | ||
| - name: JOBID | ||
| generate: expression | ||
| from: "[0-9a-z]{7}" | ||
| - name: IMAGE_TAG | ||
| value: '' | ||
| required: true | ||
| - name: LOG_BUCKET | ||
| value: 'osde2e-logs' | ||
| - name: USE_EXISTING_CLUSTER | ||
| value: 'TRUE' | ||
| - name: SLACK_NOTIFY | ||
| required: false |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in boilerplate/openshift/golang-osd-e2e/e2e-template.yml test/e2e/e2e-template.yml; do
echo "== $f =="
echo "-- occurrences of SLACK_CHANNEL placeholder --"
rg -n '\${SLACK_CHANNEL}' "$f" || true
echo "-- declarations of SLACK_CHANNEL parameter --"
rg -n '^\s*-\s*name:\s*SLACK_CHANNEL\s*$' "$f" || true
echo
doneRepository: openshift/ocm-agent-operator
Length of output: 574
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in boilerplate/openshift/golang-osd-e2e/e2e-template.yml test/e2e/e2e-template.yml; do
echo "== $f =="
echo "-- occurrences of SLACK_CHANNEL placeholder --"
rg -n 'SLACK_CHANNEL' "$f" || echo "No matches found"
echo "-- Parameter declarations section --"
rg -n -A 2 'parameters:' "$f" | head -40
echo
doneRepository: openshift/ocm-agent-operator
Length of output: 549
🏁 Script executed:
#!/bin/bash
# Show the full parameters section for both files
for f in boilerplate/openshift/golang-osd-e2e/e2e-template.yml test/e2e/e2e-template.yml; do
echo "=== $f parameters section ==="
rg -A 50 'parameters:' "$f" | head -60
echo
doneRepository: openshift/ocm-agent-operator
Length of output: 2655
Missing SLACK_CHANNEL parameter while still referencing ${SLACK_CHANNEL}.
Both boilerplate/openshift/golang-osd-e2e/e2e-template.yml and test/e2e/e2e-template.yml reference ${SLACK_CHANNEL} in the slackChannel field (line 99), but the parameter is not declared in the parameters section. Add the missing parameter or remove the interpolation.
🔧 Proposed fix (re-add missing parameter)
parameters:
- name: OSDE2E_CONFIGS
required: true
- name: TEST_IMAGE
required: true
+- name: SLACK_CHANNEL
+ required: false
- name: OCM_CCS
required: false
- name: CLOUD_PROVIDER_REGION
required: false📝 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.
Suggested change
| - name: OSDE2E_CONFIGS | |
| required: true | |
| - name: TEST_IMAGE | |
| required: true | |
| - name: OCM_CCS | |
| required: false | |
| - name: CLOUD_PROVIDER_REGION | |
| required: false | |
| - name: JOBID | |
| generate: expression | |
| from: "[0-9a-z]{7}" | |
| - name: IMAGE_TAG | |
| value: '' | |
| required: true | |
| - name: LOG_BUCKET | |
| value: 'osde2e-logs' | |
| - name: USE_EXISTING_CLUSTER | |
| value: 'TRUE' | |
| - name: SLACK_NOTIFY | |
| required: false | |
| - name: OSDE2E_CONFIGS | |
| required: true | |
| - name: TEST_IMAGE | |
| required: true | |
| - name: SLACK_CHANNEL | |
| required: false | |
| - name: OCM_CCS | |
| required: false | |
| - name: CLOUD_PROVIDER_REGION | |
| required: false | |
| - name: JOBID | |
| generate: expression | |
| from: "[0-9a-z]{7}" | |
| - name: IMAGE_TAG | |
| value: '' | |
| required: true | |
| - name: LOG_BUCKET | |
| value: 'osde2e-logs' | |
| - name: USE_EXISTING_CLUSTER | |
| value: 'TRUE' | |
| - name: SLACK_NOTIFY | |
| required: false |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@boilerplate/openshift/golang-osd-e2e/e2e-template.yml` around lines 7 - 26,
The parameters block is missing a declaration for SLACK_CHANNEL while the
template still references ${SLACK_CHANNEL} in the slackChannel field; add a
parameter entry named SLACK_CHANNEL (e.g., "name: SLACK_CHANNEL" and set
appropriate required/value like other params) to
boilerplate/openshift/golang-osd-e2e/e2e-template.yml (and mirror the same
addition in test/e2e/e2e-template.yml) so the interpolation resolves at runtime.
Comment on lines
+441
to
+452
| "config": { | ||
| "openAPIV3Schema": { | ||
| "properties": { | ||
| "image": { | ||
| "description": "Operator image to deploy", | ||
| "type": "string", | ||
| "default": "None", | ||
| }, | ||
| "type": "object", | ||
| } | ||
| } | ||
| }, |
There was a problem hiding this comment.
Misplaced type: object in OpenAPI schema structure.
The "type": "object" is incorrectly nested inside "properties" instead of being a sibling. This produces an invalid OpenAPI V3 schema in the generated PackageManifest.
🐛 Proposed fix
"config": {
"openAPIV3Schema": {
+ "type": "object",
"properties": {
"image": {
"description": "Operator image to deploy",
"type": "string",
"default": "None",
},
- "type": "object",
}
}
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@boilerplate/openshift/golang-osd-operator/olm_pko_migration.py` around lines
441 - 452, The OpenAPI schema places "type": "object" inside "properties"
instead of alongside it; modify the dict under "config" -> "openAPIV3Schema" so
that "properties" (which includes "image") remains as-is and the "type":
"object" entry is moved out to be a sibling of "properties" (i.e.,
openAPIV3Schema should contain both "type": "object" and "properties": {
"image": { ... } }), ensuring the generated PackageManifest has a valid OpenAPI
V3 schema.
Comment on lines
+274
to
+281
| @echo "Setting up kubebuilder test assets..."; \ | ||
| if ! ASSETS_PATH=$$($(SETUP_ENVTEST) use $(ENVTEST_K8S_VERSION) --arch amd64 --os linux --bin-dir /tmp/envtest-binaries -p path 2>&1); then \ | ||
| echo "ERROR: Could not obtain kubebuilder test assets"; \ | ||
| echo "Output: $$ASSETS_PATH"; \ | ||
| exit 1; \ | ||
| fi; \ | ||
| echo "Using test assets: $$ASSETS_PATH"; \ | ||
| ${GOENV} KUBEBUILDER_ASSETS="$$ASSETS_PATH" go test $(TESTOPTS) $(TESTTARGETS) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# First, let's look at the specific lines mentioned
echo "=== Lines 274-281 from standard.mk ==="
sed -n '274,281p' boilerplate/openshift/golang-osd-operator/standard.mk
echo -e "\n=== Searching for GOOS, GOARCH, setup-envtest patterns ==="
rg -n 'GOOS|GOARCH|setup-envtest|--arch|--os' \
boilerplate/openshift/golang-osd-operator/standard.mk -C2Repository: openshift/ocm-agent-operator
Length of output: 2099
go-test is hardcoded to linux/amd64 assets.
This makes local test execution fail on non-linux or non-amd64 hosts. The Makefile already defines $(GOOS) and $(GOARCH) variables (lines 93–94) that should be used instead of fixed values.
🔧 Proposed fix
- if ! ASSETS_PATH=$$($(SETUP_ENVTEST) use $(ENVTEST_K8S_VERSION) --arch amd64 --os linux --bin-dir /tmp/envtest-binaries -p path 2>&1); then \
+ if ! ASSETS_PATH=$$($(SETUP_ENVTEST) use $(ENVTEST_K8S_VERSION) --arch $(GOARCH) --os $(GOOS) --bin-dir /tmp/envtest-binaries -p path 2>&1); then \📝 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.
Suggested change
| @echo "Setting up kubebuilder test assets..."; \ | |
| if ! ASSETS_PATH=$$($(SETUP_ENVTEST) use $(ENVTEST_K8S_VERSION) --arch amd64 --os linux --bin-dir /tmp/envtest-binaries -p path 2>&1); then \ | |
| echo "ERROR: Could not obtain kubebuilder test assets"; \ | |
| echo "Output: $$ASSETS_PATH"; \ | |
| exit 1; \ | |
| fi; \ | |
| echo "Using test assets: $$ASSETS_PATH"; \ | |
| ${GOENV} KUBEBUILDER_ASSETS="$$ASSETS_PATH" go test $(TESTOPTS) $(TESTTARGETS) | |
| `@echo` "Setting up kubebuilder test assets..."; \ | |
| if ! ASSETS_PATH=$$($(SETUP_ENVTEST) use $(ENVTEST_K8S_VERSION) --arch $(GOARCH) --os $(GOOS) --bin-dir /tmp/envtest-binaries -p path 2>&1); then \ | |
| echo "ERROR: Could not obtain kubebuilder test assets"; \ | |
| echo "Output: $$ASSETS_PATH"; \ | |
| exit 1; \ | |
| fi; \ | |
| echo "Using test assets: $$ASSETS_PATH"; \ | |
| ${GOENV} KUBEBUILDER_ASSETS="$$ASSETS_PATH" go test $(TESTOPTS) $(TESTTARGETS) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@boilerplate/openshift/golang-osd-operator/standard.mk` around lines 274 -
281, The make recipe hardcodes linux/amd64 when invoking $(SETUP_ENVTEST) which
breaks non-linux/amd64 hosts; update the invocation to use the existing Make
vars $(GOOS) and $(GOARCH) instead of literal "linux" and "amd64" so ASSETS_PATH
is obtained for the current platform (affecting the code around ASSETS_PATH
assignment and the $(SETUP_ENVTEST) call that uses $(ENVTEST_K8S_VERSION)); keep
the rest of the logic (error handling, echo, and exporting KUBEBUILDER_ASSETS to
the ${GOENV} go test invocation) unchanged.
Comment on lines
+432
to
+444
| .PHONY: pko-migrate-no-dockerfile | ||
| pko-migrate-no-dockerfile: ## Migrate to PKO without generating Dockerfile | ||
| @python3 ${CONVENTION_DIR}/olm_pko_migration.py \ | ||
| --folder deploy \ | ||
| --output deploy_pko \ | ||
| --no-dockerfile | ||
|
|
||
| .PHONY: pko-migrate-no-tekton | ||
| pko-migrate-no-tekton: ## Migrate to PKO without generating Tekton pipelines | ||
| @python3 ${CONVENTION_DIR}/olm_pko_migration.py \ | ||
| --folder deploy \ | ||
| --output deploy_pko \ | ||
| --no-tekton |
There was a problem hiding this comment.
Add the same script-existence guard to the variant migration targets.
pko-migrate-no-dockerfile and pko-migrate-no-tekton should mirror the check used in pko-migrate to keep failure behavior consistent.
🔧 Proposed fix
.PHONY: pko-migrate-no-dockerfile
pko-migrate-no-dockerfile: ## Migrate to PKO without generating Dockerfile
+ `@if` [ ! -f "${CONVENTION_DIR}/olm_pko_migration.py" ]; then \
+ echo "ERROR: Migration script not found at ${CONVENTION_DIR}/olm_pko_migration.py"; \
+ exit 1; \
+ fi
`@python3` ${CONVENTION_DIR}/olm_pko_migration.py \
--folder deploy \
--output deploy_pko \
--no-dockerfile
.PHONY: pko-migrate-no-tekton
pko-migrate-no-tekton: ## Migrate to PKO without generating Tekton pipelines
+ `@if` [ ! -f "${CONVENTION_DIR}/olm_pko_migration.py" ]; then \
+ echo "ERROR: Migration script not found at ${CONVENTION_DIR}/olm_pko_migration.py"; \
+ exit 1; \
+ fi
`@python3` ${CONVENTION_DIR}/olm_pko_migration.py \
--folder deploy \
--output deploy_pko \
--no-tekton📝 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.
Suggested change
| .PHONY: pko-migrate-no-dockerfile | |
| pko-migrate-no-dockerfile: ## Migrate to PKO without generating Dockerfile | |
| @python3 ${CONVENTION_DIR}/olm_pko_migration.py \ | |
| --folder deploy \ | |
| --output deploy_pko \ | |
| --no-dockerfile | |
| .PHONY: pko-migrate-no-tekton | |
| pko-migrate-no-tekton: ## Migrate to PKO without generating Tekton pipelines | |
| @python3 ${CONVENTION_DIR}/olm_pko_migration.py \ | |
| --folder deploy \ | |
| --output deploy_pko \ | |
| --no-tekton | |
| .PHONY: pko-migrate-no-dockerfile | |
| pko-migrate-no-dockerfile: ## Migrate to PKO without generating Dockerfile | |
| `@if` [ ! -f "${CONVENTION_DIR}/olm_pko_migration.py" ]; then \ | |
| echo "ERROR: Migration script not found at ${CONVENTION_DIR}/olm_pko_migration.py"; \ | |
| exit 1; \ | |
| fi | |
| `@python3` ${CONVENTION_DIR}/olm_pko_migration.py \ | |
| --folder deploy \ | |
| --output deploy_pko \ | |
| --no-dockerfile | |
| .PHONY: pko-migrate-no-tekton | |
| pko-migrate-no-tekton: ## Migrate to PKO without generating Tekton pipelines | |
| `@if` [ ! -f "${CONVENTION_DIR}/olm_pko_migration.py" ]; then \ | |
| echo "ERROR: Migration script not found at ${CONVENTION_DIR}/olm_pko_migration.py"; \ | |
| exit 1; \ | |
| fi | |
| `@python3` ${CONVENTION_DIR}/olm_pko_migration.py \ | |
| --folder deploy \ | |
| --output deploy_pko \ | |
| --no-tekton |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@boilerplate/openshift/golang-osd-operator/standard.mk` around lines 432 -
444, The two Make targets pko-migrate-no-dockerfile and pko-migrate-no-tekton
currently run ${CONVENTION_DIR}/olm_pko_migration.py unguarded; add the same
script-existence guard used by the pko-migrate target so they first check that
${CONVENTION_DIR}/olm_pko_migration.py exists and print a clear error/exit if
missing, then run the python invocation; update the targets
pko-migrate-no-dockerfile and pko-migrate-no-tekton to mirror the guard logic
and messaging used by pko-migrate.
Contributor
|
/lgtm |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: TheUndeadKing, tkong-redhat 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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
Double the ocm-agent memory limits. 64Mi is quite low in the current situation
What this PR does / why we need it?
ocm-agent memory limits is 64Mi is quite low ATM. Easily cause OOM
Which Jira/Github issue(s) this PR fixes?
Fixes #
Special notes for your reviewer:
Pre-checks (if applicable):
make generatecommand locally to validate code changesSummary by CodeRabbit