Improve Makefile performance and reduce spurious bundle changes #1984
Conversation
openshift-ci
bot
added
the
approved
| OLD_CREATEDAT=$$(grep '^ createdAt: ' /tmp/oadp-old-csv.yaml); \ | ||
| NEW_CREATEDAT=$$(grep '^ createdAt: ' bundle/manifests/oadp-operator.clusterserviceversion.yaml); \ | ||
| cp bundle/manifests/oadp-operator.clusterserviceversion.yaml /tmp/oadp-new-csv-with-old-timestamp.yaml; \ | ||
| $(SED) -i "s/^ createdAt: .*/$$OLD_CREATEDAT/" /tmp/oadp-new-csv-with-old-timestamp.yaml; \ |
There was a problem hiding this comment.
Just noting that sed -i would work here only with gsed on macos hence the prior > + mv
Lines 422 to 438 in b9229e4
There was a problem hiding this comment.
Anyone w macos once installing gsed should be good to go.
There was a problem hiding this comment.
@kaovilai that part was not chaged (discovery of SED on MacOS X), could you please check if the reworked Makefile requires some changes to work in it, or we are fine here?
|
/test images |
|
/test 4.19-e2e-test-cli-aws |
|
I'm working to fix 4.19-e2e-test-cli-aws seperately... can override 4.19-e2e-test-cli-aws |
|
/test images |
1 similar comment
|
/test images |
|
/retest |
weshayutin
left a comment
weshayutin
left a comment
There was a problem hiding this comment.
this all worked for me.. thank you!
|
/retest |
1 similar comment
|
/retest |
openshift-merge-robot
added
the
needs-rebase
|
rebase! |
|
@mpryc this is a good pr, let's get er done |
mpryc
force-pushed
the
makefile_rework_for_rebasebot
branch
from
c6c2fdd to
34fd433
Compare
openshift-merge-robot
removed
the
needs-rebase
WalkthroughAdded a new public Makefile target Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**⚙️ CodeRabbit configuration file
Files:
🔇 Additional comments (13)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mpryc The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
2 similar comments
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mpryc The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mpryc The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
Makefile (1)
377-382: Same issue as docker-build: defensive check needed for CLUSTER_OS and CLUSTER_ARCH.See the comment for
docker-build(lines 261–266). Apply the same defensive check tobundle-build.
🧹 Nitpick comments (1)
Makefile (1)
787-831: Runtime variable computation is correct but complex.Deferring cloud-specific variable evaluation to target invocation time (rather than parse time) achieves the performance goal and enables rebasebot to run bundle targets without cluster access. The bash logic is sound but dense; for future maintainability, consider extracting this logic into a dedicated shell script (e.g.,
tests/e2e/scripts/setup_runtime_vars.sh) to simplify the Makefile and improve testability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
Makefile(10 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
Makefile
🔇 Additional comments (6)
Makefile (6)
93-99: Lazy container tool validation is well-implemented.Deferring the tool existence check to runtime (only when targets that need it are invoked) avoids spurious failures during parse-time evaluation (e.g.,
make help). The error message is clear, and the conditional logic correctly detects absence.
248-252: Verify downstream code handles empty CLUSTER_TYPE, CLUSTER_OS, and CLUSTER_ARCH.The
timeout 2wrapper prevents hanging when not connected to a cluster, which is essential for the rebasebot use case. However, if the timeout expires, these variables will be empty strings. Confirm that conditional logic indocker-build,bundle-build, andtest-e2e-setupsafely handles empty values without generating invalid build arguments or shell syntax errors.
349-374: CSV timestamp preservation is well-implemented.The logic correctly detects when only the
createdAttimestamp has changed and preserves the old value, reducing spurious bundle diffs. The temp file handling is clean, and the use of$(SED)is correct for runtime evaluation. The approach assumes consistent YAML formatting for thecreatedAtfield; if this assumption breaks in the future, a YAML-aware tool (likeyq, which is already available in the Makefile) could replace sed-based manipulation.
547-552: Runtime deferral of CLUSTER_TYPE check is correct.Moving the cluster login validation from parse time to target invocation time prevents spurious failures during
make helpand aligns with the performance goal of this PR.
334-334: Operator-SDK and OPM version checks correctly suppress errors for missing binaries.The conditions now only download tools if they don't exist or the version doesn't match, without emitting spurious error messages. This aligns with the PR goal of quieting tool-version warnings when binaries aren't yet installed.
Also applies to: 391-391
419-419: Catalog build/push dependencies are correct.Adding
check-container-toolensures the container tool is validated before attempting catalog operations.Also applies to: 424-424
mpryc
force-pushed
the
makefile_rework_for_rebasebot
branch
from
34fd433 to
3dc6361
Compare
Change that makes rebase of oadp-operator possible:
- Make `make help` instant by deferring CLUSTER_TYPE evaluation to runtime
(was 12s, now 0.06s - 200x faster)
- Move cloud-specific variables to test-e2e targets to avoid parse-time
cluster API calls that hang when not connected
- Preserve createdAt timestamp in CSV when only timestamp changed
- Suppress tool version check warnings when binaries don't exist yet
- Make container tool check lazy (only runs when needed)
- Clean up commented code with concise explanatory notes
Signed-off-by: Michal Pryc <[email protected]>
Co-Authored-By: Claude <[email protected]>
mpryc
force-pushed
the
makefile_rework_for_rebasebot
branch
from
3dc6361 to
56659c0
Compare
|
/retest |
|
@mpryc: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
openshift-merge-robot
added
the
needs-rebase
|
PR needs rebase. 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. |
|
/retest ai-retester: The e2e test comment for /pull/1984 |
4 participants
Change that makes rebase of oadp-operator possible:
make helpinstant by deferring CLUSTER_TYPE evaluation to runtime (was 12s, now 0.06s - 200x faster)Why the changes were made
To allow rebasebot properly run $ make bundle, without access to the cluster.
Without this change it's impossible to update manifests without being logged to the cluster.
How to test the changes made
Reading Makefile, asking right questions to claude and modifying code by hand + testing with few make targets:
$ make help
$ make manifests
$ make build
$ make bundle
$ make deploy-olm
$ make run
$ make install
$ make undeploy-olm
$ make uninstall
Tests should be performed by prow CI to execute some other make targets.