Add the install.sh script and upload it to S3#54
Conversation
Copy the install.sh script over from c/c, since its source of truth should probably be this repository. Update CI to push the script to our S3 bucket, so that if we update it in the future it automatically ends up there. Signed-off-by: Adam Wolfe Gordon <awg@upbound.io>
📝 WalkthroughWalkthroughThis PR introduces ChangesCrossplane CLI Installation Script
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![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
🧹 Nitpick comments (1)
install.sh (1)
100-103: ⚡ Quick winConsider adding a timeout to the curl command for better reliability.
While the current flags are appropriate, adding
--max-time 300(or a suitable timeout) would prevent the script from hanging indefinitely on slow or stalled network connections, improving the user experience.⏱️ Proposed enhancement with timeout
-if ! curl -sfL "${url}" -o "${url_file}"; then +if ! curl -sfL --max-time 300 "${url}" -o "${url_file}"; then echo "Failed to download Crossplane CLI. Please make sure ${url_error}version ${XP_VERSION} exists on channel ${XP_CHANNEL}." exit 1 fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@install.sh` around lines 100 - 103, Add a maximum-time to the curl invocation to avoid hangs: modify the curl command that downloads "${url}" into "${url_file}" (the block referencing variables url, url_file, url_error, XP_VERSION, XP_CHANNEL) to include a suitable timeout flag such as --max-time 300 (or another chosen seconds value) so the download fails fast and the existing error message/exit path is exercised when the timeout is reached.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@install.sh`:
- Around line 105-111: The script currently unconditionally runs rm
"${BIN}.sha256" after extracting ${url_file}, which will cause the script to
fail under set -e if the checksum file is absent; either make the removal safe
by using rm -f "${BIN}.sha256" (quick fix) or, preferably, validate the checksum
before deleting by checking for the presence of "${BIN}.sha256", verifying it
with sha256sum -c (or similar) against the extracted binary, fail on mismatch,
and only then remove the checksum file; update the block that handles extraction
of ${url_file} and the subsequent rm to implement one of these two approaches.
---
Nitpick comments:
In `@install.sh`:
- Around line 100-103: Add a maximum-time to the curl invocation to avoid hangs:
modify the curl command that downloads "${url}" into "${url_file}" (the block
referencing variables url, url_file, url_error, XP_VERSION, XP_CHANNEL) to
include a suitable timeout flag such as --max-time 300 (or another chosen
seconds value) so the download fails fast and the existing error message/exit
path is exercised when the timeout is reached.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ea08c642-f644-44d2-8d4e-bcebaf9d9282
⛔ Files ignored due to path filters (1)
nix/apps.nixis excluded by none and included by none
📒 Files selected for processing (1)
install.sh
| if [ "${_compr}" = "true" ]; then | ||
| if ! tar xzf "${url_file}"; then | ||
| echo "Failed to unpack the Crossplane CLI compressed file." | ||
| exit 1 | ||
| fi | ||
| rm "${BIN}.sha256" "${url_file}" | ||
| fi |
There was a problem hiding this comment.
Fix potential failure when .sha256 file is missing from tarball.
Line 110 unconditionally removes ${BIN}.sha256, but with set -e in effect, the script will exit if this file doesn't exist in the extracted tarball. This could break installations if the tarball structure changes.
Additionally, while the script removes the checksum file, it never validates it—a missed security opportunity. Would you like to either:
- Add checksum validation before removal, or
- Use
rm -fto ignore missing files?
🛡️ Proposed fix options
Option 1: Make rm fail-safe (quick fix)
- rm "${BIN}.sha256" "${url_file}"
+ rm -f "${BIN}.sha256" "${url_file}"Option 2: Validate checksum before removal (more secure)
+ if [ -f "${BIN}.sha256" ]; then
+ if ! sha256sum -c "${BIN}.sha256"; then
+ echo "Checksum validation failed for Crossplane CLI."
+ exit 1
+ fi
+ fi
rm -f "${BIN}.sha256" "${url_file}"📝 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.
| if [ "${_compr}" = "true" ]; then | |
| if ! tar xzf "${url_file}"; then | |
| echo "Failed to unpack the Crossplane CLI compressed file." | |
| exit 1 | |
| fi | |
| rm "${BIN}.sha256" "${url_file}" | |
| fi | |
| if [ "${_compr}" = "true" ]; then | |
| if ! tar xzf "${url_file}"; then | |
| echo "Failed to unpack the Crossplane CLI compressed file." | |
| exit 1 | |
| fi | |
| rm -f "${BIN}.sha256" "${url_file}" | |
| fi |
| if [ "${_compr}" = "true" ]; then | |
| if ! tar xzf "${url_file}"; then | |
| echo "Failed to unpack the Crossplane CLI compressed file." | |
| exit 1 | |
| fi | |
| rm "${BIN}.sha256" "${url_file}" | |
| fi | |
| if [ "${_compr}" = "true" ]; then | |
| if ! tar xzf "${url_file}"; then | |
| echo "Failed to unpack the Crossplane CLI compressed file." | |
| exit 1 | |
| fi | |
| if [ -f "${BIN}.sha256" ]; then | |
| if ! sha256sum -c "${BIN}.sha256"; then | |
| echo "Checksum validation failed for Crossplane CLI." | |
| exit 1 | |
| fi | |
| fi | |
| rm -f "${BIN}.sha256" "${url_file}" | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@install.sh` around lines 105 - 111, The script currently unconditionally runs
rm "${BIN}.sha256" after extracting ${url_file}, which will cause the script to
fail under set -e if the checksum file is absent; either make the removal safe
by using rm -f "${BIN}.sha256" (quick fix) or, preferably, validate the checksum
before deleting by checking for the presence of "${BIN}.sha256", verifying it
with sha256sum -c (or similar) against the extracted binary, fail on mismatch,
and only then remove the checksum file; update the block that handles extraction
of ${url_file} and the subsequent rm to implement one of these two approaches.
There was a problem hiding this comment.
I believe we should leave it as it is. If the checksum file is missing from the tarball, then we have a bigger problem to solve. Also, I'd like to introduce the validation functionality in a separate PR
There was a problem hiding this comment.
@tampakrap, that's a fair point — failing loudly when .sha256 is absent is actually the safer behavior, since it signals a malformed release artifact rather than silently continuing. Happy to defer the full validation to a separate PR.
Would you like me to open a GitHub issue to track the checksum validation work so it doesn't get lost?
|
are we changing the docs or not yet? |
I have an update ready for PR. Was going to wait until this one is merged, but I did already manually upload |
|
@tampakrap Docs update in crossplane/docs#1099 :-). |
tampakrap
left a comment
tampakrap
left a comment
There was a problem hiding this comment.
thanks, please add the docs link in the PR description as well
2 participants
Description of your changes
Copy the install.sh script over from c/c, since its source of truth should probably be this repository. Update CI to push the script to our S3 bucket, so that if we update it in the future it automatically ends up there. Note that we upload install.sh from the
promote-artifactsjob only from the main branch.Related docs change: crossplane/docs#1099
I have:
./nix.sh flake checkto ensure this PR is ready for review.- [ ] Added or updated unit tests.- [ ] Addedbackport release-x.ylabels to auto-backport this PR.