-
Notifications
You must be signed in to change notification settings - Fork 5
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
Use DDEV method for SQLite to avoid side effects of debian testing #14
Conversation
WalkthroughThe pull request introduces architectural flexibility and improved SQLite installation in the Dockerfile. By adding Changes
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/Dockerfile (1)
51-57
: Consider parameterizing the snapshot dateThe implementation successfully avoids debian testing side effects by using specific version downloads. However, the snapshot date is hardcoded:
20240203T152533ZConsider making this a build argument for easier updates when newer versions are released.
ARG SQLITE_VERSION="3.45.1" +ARG DEBIAN_SNAPSHOT_DATE="20240203T152533Z" RUN if [ "$INSTALL_SQLITE" = "true" ]; then \ mkdir -p /tmp/sqlite3; \ - wget -O /tmp/sqlite3/sqlite3.deb https://snapshot.debian.org/archive/debian/20240203T152533Z/pool/main/s/sqlite3/sqlite3_${SQLITE_VERSION}-1_${TARGETARCH}.deb; \ - wget -O /tmp/sqlite3/libsqlite3.deb https://snapshot.debian.org/archive/debian/20240203T152533Z/pool/main/s/sqlite3/libsqlite3-0_${SQLITE_VERSION}-1_${TARGETARCH}.deb; \ + wget -O /tmp/sqlite3/sqlite3.deb https://snapshot.debian.org/archive/debian/${DEBIAN_SNAPSHOT_DATE}/pool/main/s/sqlite3/sqlite3_${SQLITE_VERSION}-1_${TARGETARCH}.deb; \ + wget -O /tmp/sqlite3/libsqlite3.deb https://snapshot.debian.org/archive/debian/${DEBIAN_SNAPSHOT_DATE}/pool/main/s/sqlite3/libsqlite3-0_${SQLITE_VERSION}-1_${TARGETARCH}.deb; \
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Dockerfile
(2 hunks)
🧰 Additional context used
🪛 Hadolint (2.12.0)
src/Dockerfile
[error] 24-24: Do not use sudo as it leads to unpredictable behavior. Use a tool like gosu to enforce root
(DL3004)
🔇 Additional comments (2)
src/Dockerfile (2)
9-9
: Well-structured architecture-specific binary downloads!The introduction of
TARGETARCH
and its consistent usage across kubectl, helm, and yq installations is a significant improvement. This approach:
- Leverages Docker BuildKit's native cross-platform capabilities
- Provides more reliable architecture detection compared to runtime checks
- Ensures consistency across all architecture-dependent downloads
Also applies to: 16-22, 24-25, 43-44
48-51
: Well-documented implementation choice!The comments effectively explain the rationale for choosing the DDEV method and reference the source implementation. This helps future maintainers understand the design decision.
RUN curl -s https://get.helm.sh/helm-v3.6.2-linux-${TARGETARCH}.tar.gz | sudo tar -C /tmp --no-same-owner -xvzf - linux-${TARGETARCH}/helm --strip-components 1 && \ | ||
mv /tmp/helm /bin/helm3 |
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.
Remove sudo usage in helm3 installation
Using sudo in Dockerfile RUN commands can lead to unpredictable behavior. Consider modifying the command to avoid sudo:
-RUN curl -s https://get.helm.sh/helm-v3.6.2-linux-${TARGETARCH}.tar.gz | sudo tar -C /tmp --no-same-owner -xvzf - linux-${TARGETARCH}/helm --strip-components 1 && \
+RUN curl -s https://get.helm.sh/helm-v3.6.2-linux-${TARGETARCH}.tar.gz | tar -C /tmp --no-same-owner -xvzf - linux-${TARGETARCH}/helm --strip-components 1 && \
mv /tmp/helm /bin/helm3
📝 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.
RUN curl -s https://get.helm.sh/helm-v3.6.2-linux-${TARGETARCH}.tar.gz | sudo tar -C /tmp --no-same-owner -xvzf - linux-${TARGETARCH}/helm --strip-components 1 && \ | |
mv /tmp/helm /bin/helm3 | |
RUN curl -s https://get.helm.sh/helm-v3.6.2-linux-${TARGETARCH}.tar.gz | tar -C /tmp --no-same-owner -xvzf - linux-${TARGETARCH}/helm --strip-components 1 && \ | |
mv /tmp/helm /bin/helm3 |
🧰 Tools
🪛 Hadolint (2.12.0)
[error] 24-24: Do not use sudo as it leads to unpredictable behavior. Use a tool like gosu to enforce root
(DL3004)
💡 Codebase verification
🛠️ Refactor suggestion
Checksum verification is recommended and available for helm binary
Based on the verification results:
- Helm provides SHA256 checksums for both amd64 and arm64 architectures (HTTP 200 responses)
- YQ has a general checksums file available in the release assets
- For SQLite packages from Debian, checksums are typically included in the .dsc file and can be verified using apt's built-in mechanisms
To implement checksum verification:
- For helm: Download and verify the SHA256 checksum before installing:
RUN curl -sLO "https://get.helm.sh/helm-v3.6.2-linux-${TARGETARCH}.tar.gz.sha256sum" && \
curl -sL "https://get.helm.sh/helm-v3.6.2-linux-${TARGETARCH}.tar.gz" | tee helm.tar.gz | sha256sum -c helm-v3.6.2-linux-${TARGETARCH}.tar.gz.sha256sum && \
tar -C /tmp --no-same-owner -xvzf helm.tar.gz linux-${TARGETARCH}/helm --strip-components 1 && \
mv /tmp/helm /bin/helm3
- For yq: Download and verify against the checksums file
- For SQLite: Use apt's built-in verification mechanisms when installing the package
🔗 Analysis chain
Consider adding checksum verification for binary downloads
For security, consider adding checksum verification for helm3, yq, and SQLite package downloads. This helps ensure the integrity of downloaded binaries.
Also applies to: 43-44, 54-55
Let me try a different approach to verify the checksums availability.
Let me try one more verification for yq checksums with a different pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if checksums are available for the downloaded binaries
echo "Checking helm3 checksums..."
curl -sI "https://get.helm.sh/helm-v3.6.2-linux-${TARGETARCH}.tar.gz.sha256sum"
echo "Checking yq checksums..."
curl -sI "https://github.com/mikefarah/yq/releases/download/v4.21.1/yq_linux_${TARGETARCH}.tar.gz.sha256"
echo "Checking SQLite package checksums..."
curl -sI "https://snapshot.debian.org/archive/debian/20240203T152533Z/pool/main/s/sqlite3/sqlite3_3.45.1-1_${TARGETARCH}.deb.sha256"
Length of output: 2015
Script:
#!/bin/bash
# Check if checksums are available for the downloaded binaries with correct architecture patterns
echo "Checking helm3 checksums..."
curl -sI "https://get.helm.sh/helm-v3.6.2-linux-amd64.tar.gz.sha256sum"
curl -sI "https://get.helm.sh/helm-v3.6.2-linux-arm64.tar.gz.sha256sum"
echo -e "\nChecking yq checksums..."
curl -sI "https://github.com/mikefarah/yq/releases/download/v4.21.1/yq_linux_amd64.tar.gz.sha256"
curl -sI "https://github.com/mikefarah/yq/releases/download/v4.21.1/yq_linux_arm64.tar.gz.sha256"
echo -e "\nChecking SQLite package checksums..."
curl -sI "https://snapshot.debian.org/archive/debian/20240203T152533Z/pool/main/s/sqlite3/sqlite3_3.45.1-1_amd64.deb.sha256"
curl -sI "https://snapshot.debian.org/archive/debian/20240203T152533Z/pool/main/s/sqlite3/sqlite3_3.45.1-1_arm64.deb.sha256"
Length of output: 4344
Script:
#!/bin/bash
# Check if checksums are available for yq with different patterns
echo "Checking yq checksums..."
curl -sI "https://github.com/mikefarah/yq/releases/download/v4.21.1/checksums"
curl -sI "https://github.com/mikefarah/yq/releases/download/v4.21.1/checksums.txt"
# Also check if SQLite has checksums in a different format
echo -e "\nChecking SQLite checksums..."
curl -sI "https://snapshot.debian.org/archive/debian/20240203T152533Z/pool/main/s/sqlite3/sqlite3_3.45.1-1.dsc"
Length of output: 6122
🧰 Tools
🪛 Hadolint (2.12.0)
[error] 24-24: Do not use sudo as it leads to unpredictable behavior. Use a tool like gosu to enforce root
(DL3004)
Summary by CodeRabbit
kubectl
,helm
, andyq