Fix ContainerfilesDirect build instead of make targets to catch failures#180
Fix ContainerfilesDirect build instead of make targets to catch failures#180Joeavaikath wants to merge 1 commit intomigtools:oadp-devfrom
Conversation
Signed-off-by: Joseph <jvaikath@redhat.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Joeavaikath 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 |
|
/cherry-pick oadp-1.6 |
📝 WalkthroughWalkthroughThe pull request updates two Dockerfiles to replace single Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
|
@Joeavaikath: once the present PR merges, I will cherry-pick it on top of 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. |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Containerfile.download`:
- Line 41: The checksum file currently contains absolute paths because the line
invoking sha256sum uses /archives/$output; change the invocation so sha256sum
computes checksums using relative filenames (e.g., run sha256sum from inside the
/archives directory or otherwise pass just $output as the filename) and write
the result to /archives/$output.sha256; update the sha256sum invocation that
references /archives/$output and $output.sha256 to ensure the output uses the
bare filename so downstream sha256sum -c works for users.
In `@konflux.Dockerfile`:
- Line 35: The checksum line currently invokes sha256sum on the absolute path
and writes an absolute-path filename (causing mismatch for users); update the
step that runs sha256sum on the $output artifact so it records the basename
rather than the absolute path and writes to a $output.sha256 file in the same
folder — achieve this by running sha256sum on the relative filename (or changing
into the archives directory first) so the produced checksum file contains the
plain filename and will verify with sha256sum -c; target the existing sha256sum
invocation that references $output and the generated .sha256 file.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0fb0a841-3f5f-4f31-a858-e015c63cb352
📒 Files selected for processing (2)
Containerfile.downloadkonflux.Dockerfile
| -X github.com/vmware-tanzu/velero/pkg/buildinfo.GitTreeState=clean" \ | ||
| -o /archives/$output \ | ||
| . && \ | ||
| sha256sum /archives/$output > /archives/$output.sha256; \ |
There was a problem hiding this comment.
Checksum files will contain absolute paths, breaking user verification.
sha256sum /archives/$output produces output like abc123... /archives/kubectl-oadp_linux_amd64. When users download the binary and checksum file, running sha256sum -c kubectl-oadp_linux_amd64.sha256 will fail because the path doesn't match.
🔧 Proposed fix to use relative paths in checksum files
- sha256sum /archives/$output > /archives/$output.sha256; \
+ (cd /archives && sha256sum $output > $output.sha256); \📝 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.
| sha256sum /archives/$output > /archives/$output.sha256; \ | |
| (cd /archives && sha256sum $output > $output.sha256); \ |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Containerfile.download` at line 41, The checksum file currently contains
absolute paths because the line invoking sha256sum uses /archives/$output;
change the invocation so sha256sum computes checksums using relative filenames
(e.g., run sha256sum from inside the /archives directory or otherwise pass just
$output as the filename) and write the result to /archives/$output.sha256;
update the sha256sum invocation that references /archives/$output and
$output.sha256 to ensure the output uses the bare filename so downstream
sha256sum -c works for users.
| -X github.com/vmware-tanzu/velero/pkg/buildinfo.GitTreeState=clean" \ | ||
| -o /archives/$output \ | ||
| . && \ | ||
| sha256sum /archives/$output > /archives/$output.sha256; \ |
There was a problem hiding this comment.
Checksum file contains absolute path, breaking user verification.
The sha256sum /archives/$output command outputs the full path (e.g., /archives/kubectl-oadp_linux_amd64) in the checksum file. When users download these files and run sha256sum -c, verification will fail because the local filename won't match.
🔧 Proposed fix to use relative path in checksum
- sha256sum /archives/$output > /archives/$output.sha256; \
+ (cd /archives && sha256sum $output > $output.sha256); \📝 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.
| sha256sum /archives/$output > /archives/$output.sha256; \ | |
| (cd /archives && sha256sum $output > $output.sha256); \ |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@konflux.Dockerfile` at line 35, The checksum line currently invokes sha256sum
on the absolute path and writes an absolute-path filename (causing mismatch for
users); update the step that runs sha256sum on the $output artifact so it
records the basename rather than the absolute path and writes to a
$output.sha256 file in the same folder — achieve this by running sha256sum on
the relative filename (or changing into the archives directory first) so the
produced checksum file contains the plain filename and will verify with
sha256sum -c; target the existing sha256sum invocation that references $output
and the generated .sha256 file.
2 participants
Why the changes were made
konflux.Dockerfile successfully builds but the oadp-cli binaries are absent: make target silently fails
How to test the changes made
konflux.Dockerfile build should succeed without issue
Summary by CodeRabbit