-
Notifications
You must be signed in to change notification settings - Fork 0
Fix #5 unzip overwrite failure and add cache reuse #6
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -38,6 +38,7 @@ echo " awscli: Detected target architecture: $target_arch" | |
| BP_DIR=$(cd $(dirname ${0:-}); cd ..; pwd) | ||
|
|
||
| TARGET_DIR="$AWS_CLI_LAYER_DIR" | ||
| AWS_CLI_BIN="$TARGET_DIR/bin/aws" | ||
|
|
||
| # Map architecture to AWS CLI installer name | ||
| case "$target_arch" in | ||
|
|
@@ -55,35 +56,42 @@ esac | |
|
|
||
| AWS_CLI_URL="https://awscli.amazonaws.com/awscli-exe-linux-${awscli_arch}.zip" | ||
|
|
||
| echo " awscli: Downloading AWS CLI for ${target_arch} (${awscli_arch})" | ||
| cd "$TARGET_DIR" && curl -L "$AWS_CLI_URL" -o awscli.zip || { | ||
| echo " ! awscli: Failed to download AWS CLI for ${target_arch}" | ||
| exit 1 | ||
| } | ||
|
|
||
| cd "$TARGET_DIR" && unzip -q awscli.zip || { | ||
| echo " ! awscli: Failed to extract AWS CLI" | ||
| exit 1 | ||
| } | ||
|
|
||
| # Install AWS CLI | ||
| echo " awscli: Installing AWS CLI" | ||
| "$TARGET_DIR/aws/install" -i "$TARGET_DIR/aws-cli" -b "$TARGET_DIR/bin" || { | ||
| echo " ! awscli: Failed to install AWS CLI" | ||
| exit 1 | ||
| } | ||
|
|
||
| # Verify AWS CLI works | ||
| AWS_CLI_BIN="$TARGET_DIR/bin/aws" | ||
| if [ -f "$AWS_CLI_BIN" ]; then | ||
| # Reuse cached layer if binary exists and was built for the same architecture. | ||
| # Reading cached_arch from the TOML written by a previous build is reliable | ||
| # because the lifecycle restores it from the cache image before bin/build runs. | ||
| cached_arch="" | ||
| if [[ -f "$AWS_CLI_LAYER_TOML" ]]; then | ||
| cached_arch=$(grep 'architecture' "$AWS_CLI_LAYER_TOML" 2>/dev/null | head -1 | sed 's/.*"\(.*\)".*/\1/') | ||
| fi | ||
|
|
||
| if [[ "$cached_arch" == "$target_arch" ]] && [[ -f "$AWS_CLI_BIN" ]] && "$AWS_CLI_BIN" --version > /dev/null 2>&1; then | ||
| echo " awscli: Reusing cached AWS CLI for ${target_arch}" | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The Click to see review metadata{
"confidence": 0.95,
"severity": "minor",
"suggested_fix": "Add the `-f` flag to `curl` (e.g., `curl -fL`) so that it exits with a non-zero status code on HTTP errors.",
"verification_questions": [
"What is the current behavior if `awscli_arch` evaluates to an invalid architecture resulting in a 404?",
"Would debugging be easier if the script failed immediately at the download step with an explicit HTTP error?"
]
} |
||
| else | ||
| echo " awscli: Downloading AWS CLI for ${target_arch} (${awscli_arch})" | ||
| cd "$TARGET_DIR" && curl -L "$AWS_CLI_URL" -o awscli.zip || { | ||
| echo " ! awscli: Failed to download AWS CLI for ${target_arch}" | ||
| exit 1 | ||
| } | ||
|
|
||
| # -o: overwrite existing files without prompting (avoids EOF prompt when cache is restored) | ||
| cd "$TARGET_DIR" && unzip -qo awscli.zip || { | ||
| echo " ! awscli: Failed to extract AWS CLI" | ||
| exit 1 | ||
| } | ||
|
|
||
| echo " awscli: Installing AWS CLI" | ||
| "$TARGET_DIR/aws/install" -i "$TARGET_DIR/aws-cli" -b "$TARGET_DIR/bin" --update || { | ||
| echo " ! awscli: Failed to install AWS CLI" | ||
| exit 1 | ||
| } | ||
|
|
||
| "$AWS_CLI_BIN" --version || { | ||
| echo " ! awscli: AWS CLI verification failed" | ||
| exit 1 | ||
| } | ||
| echo " awscli: Successfully installed AWS CLI for ${target_arch}" | ||
| else | ||
| echo " ! awscli: Binary not found after installation" | ||
| exit 1 | ||
|
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The installation process leaves the extracted Click to see review metadata{
"confidence": 0.95,
"severity": "major",
"suggested_fix": "Remove the extracted installer directory by appending `rm -rf \"$TARGET_DIR/aws\"` when removing the zip file.",
"verification_questions": [
"Does `unzip awscli.zip` extract an `aws/` directory inside `TARGET_DIR`?",
"Are the files inside `TARGET_DIR/aws/` needed after the `TARGET_DIR/aws/install` script completes?"
]
} |
||
| rm -f "$TARGET_DIR/awscli.zip" | ||
| fi | ||
|
|
||
| # Expose AWS CLI on PATH at launch time. | ||
|
|
||
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.
Parsing TOML with
grepandsedcan be fragile.grep 'architecture'might accidentally match descriptions or comments containing the word 'architecture' before the actual key. Also, thesedregex strictly requires double quotes; if single quotes are used, it will fail to capture the value and fallback to the entire matched line, causing unnecessary cache misses.Click to see review metadata
{ "confidence": 0.85, "severity": "minor", "suggested_fix": "Consider anchoring the grep search (e.g., `grep -E '^\\s*architecture\\s*='`) to match only the key, and relaxing the `sed` regex to handle optional single quotes, or use a tool like `yj` or `jq` if available.", "verification_questions": [ "Could the TOML file contain the word 'architecture' in preceding lines, such as descriptions or comments?", "Does the buildpack lifecycle ever write TOML values using single quotes instead of double quotes?" ] }