fix: improve installation script by using a temporary directory for downloads #354
Conversation
|
@prasad89 I just ran and I see this error as well, is it something you could also have a look at? Thanks! |
Sure will take a look |
|
@tuannvm do you see this error all the time or just saw once? |
|
@prasad89 you are right. Now I ran again and I don't see it anymore. Looks like we need to handle the rate limit better. Any idea? |
There was a problem hiding this comment.
Pull Request Overview
This PR improves the installation script by using a temporary directory to download and extract the release tarball.
- Creates a temporary directory with a cleanup trap
- Downloads the tarball to the temporary directory and extracts it there
- Installs the binary from the temporary directory
| @@ -64,16 +64,17 @@ fi | |||
| TARBALL="kubectl-ai_${OS}_${ARCH}.tar.gz" | |||
| URL="https://github.com/$REPO/releases/download/$LATEST_TAG/$TARBALL" | |||
|
|
|||
| # Create temp dir and clean up on exit | |||
| tmpdir="$(mktemp -d)" | |||
| trap "rm -rf '$tmpdir'" EXIT | |||
There was a problem hiding this comment.
The trap command encloses $tmpdir in single quotes, which prevents variable expansion at the time the trap is executed, causing the temporary directory not to be cleaned up. Consider changing the command to: trap "rm -rf "$tmpdir"" EXIT.
| trap "rm -rf '$tmpdir'" EXIT | |
| trap "rm -rf \"$tmpdir\"" EXIT |
Copilot uses AI. Check for mistakes.
|
@tuannvm don't have any as of now, but thinking clear error message would be better something like: echo "❌ Failed to fetch latest release tag. You may have hit GitHub's rate limit."
echo "Try setting a GitHub token: export GITHUB_TOKEN=TOKEN"what are your thoughts on this? |
Previously, the install.sh script extracted the tarball into the current working directory, leaving behind files like LICENSE, README, and the binary. This PR fixes that by using a temporary directory for all downloads and extraction, ensuring no leftover files clutter the user’s workspace.