ci(docker): streamline file permissions handling #2604
Conversation
|
Warning Rate limit exceeded@steebchen has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 2 minutes and 6 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughOhayo, sensei! This pull request introduces several changes across multiple files related to Docker and GitHub workflows. The Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
Documentation and Community
|
![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
🧹 Outside diff range and nitpick comments (2)
Dockerfile (1)
29-29: Consider using explicit permissions with COPY.While permissions are handled in CI, it's a good practice to make the Dockerfile self-contained.
You can use the
--chmodflag with COPY:-COPY --from=artifacts $TARGETPLATFORM/katana $TARGETPLATFORM/sozo $TARGETPLATFORM/torii /usr/local/bin/ +COPY --from=artifacts --chmod=755 $TARGETPLATFORM/katana $TARGETPLATFORM/sozo $TARGETPLATFORM/torii /usr/local/bin/.github/workflows/release.yml (1)
138-140: Ohayo! The permission handling looks good, sensei! 🎯The addition of chmod commands in the CI workflow aligns perfectly with the PR's objective of streamlining file permissions handling. This ensures the binaries are properly executable when deployed.
For a more concise approach, consider combining the chmod commands:
- chmod +x $PLATFORM_NAME/$ARCH/katana - chmod +x $PLATFORM_NAME/$ARCH/sozo - chmod +x $PLATFORM_NAME/$ARCH/torii + chmod +x $PLATFORM_NAME/$ARCH/{katana,sozo,torii}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
.dockerignore(1 hunks).github/workflows/preview.yml(1 hunks).github/workflows/release.yml(1 hunks)Dockerfile(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .dockerignore
🧰 Additional context used
🪛 actionlint
.github/workflows/preview.yml
68-68: shellcheck reported issue in this script: SC2086:info:1:20: Double quote to prevent globbing and word splitting
(shellcheck)
68-68: shellcheck reported issue in this script: SC2086:info:2:20: Double quote to prevent globbing and word splitting
(shellcheck)
68-68: shellcheck reported issue in this script: SC2086:info:3:20: Double quote to prevent globbing and word splitting
(shellcheck)
🔇 Additional comments (3)
Dockerfile (1)
29-29: Verify binary paths in CI workflows.
The COPY command assumes specific paths in the artifacts stage.
Let's verify the binary paths in the CI workflows:
✅ Verification successful
Binary paths in CI workflows are correctly aligned with Dockerfile, sensei! ✨
Based on the CI workflows and Dockerfile analysis:
- Release workflow correctly moves binaries to
$PLATFORM_NAME/$ARCH/structure - Preview workflow places binaries in
artifacts/$PLATFORM/directory - Dockerfile's
$TARGETPLATFORMpath aligns with these CI artifacts
The COPY command's paths match the structure set up in the CI workflows, ensuring proper binary placement during container builds.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the CI workflows set up the correct binary paths
# Expected: Find references to katana, sozo, and torii in the correct paths
rg -g '*.yml' -g '*.yaml' "katana|sozo|torii" .github/workflows/
Length of output: 3133
Script:
#!/bin/bash
# Let's check the artifacts stage in the Dockerfile to verify the paths
rg -A 5 -B 5 "FROM .* as artifacts" Dockerfile
# Also check if there are any platform-specific paths being set
rg "TARGETPLATFORM|PLATFORM" Dockerfile
Length of output: 213
.github/workflows/preview.yml (1)
67-74: Verify permission handling consistency across workflows.
The chmod step aligns with the PR's objective to streamline file permissions. Let's verify this approach is consistent across all workflow files.
✅ Verification successful
Permission handling is consistent across workflows, sensei! ✨
The chmod commands are consistently applied across all workflow files:
preview.yml: Sets executable permissions for katana, sozo, and torii in artifacts/$PLATFORM/release.yml: Same pattern for $PLATFORM_NAME/$ARCH/ci.yml: Similar pattern for binaries in /tmp/bins/
All workflows follow the same approach of making the necessary binaries executable before use. The implementation in preview.yml aligns perfectly with the established patterns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent permission handling across workflows
# Expected: Similar chmod patterns in other workflow files
# Test 1: Look for chmod commands in other workflow files
echo "Checking chmod usage in workflow files:"
rg -l 'chmod \+x' .github/workflows/
# Test 2: Look for permission-related steps in other workflow files
echo "Checking for permission-related steps:"
rg -A 5 'name: .*chmod' .github/workflows/
Length of output: 707
Script:
#!/bin/bash
# Let's check the chmod commands in the other workflow files to verify consistency
echo "=== Checking chmod commands in ci.yml ==="
rg -A 5 'chmod \+x' .github/workflows/ci.yml
echo -e "\n=== Checking chmod commands in release.yml ==="
rg -A 5 'chmod \+x' .github/workflows/release.yml
# Also check if there are any other permission-related commands
echo -e "\n=== Checking for other permission commands ==="
rg -i 'permission|chmod' .github/workflows/
Length of output: 3351
🧰 Tools
🪛 actionlint
68-68: shellcheck reported issue in this script: SC2086:info:1:20: Double quote to prevent globbing and word splitting
(shellcheck)
68-68: shellcheck reported issue in this script: SC2086:info:2:20: Double quote to prevent globbing and word splitting
(shellcheck)
68-68: shellcheck reported issue in this script: SC2086:info:3:20: Double quote to prevent globbing and word splitting
(shellcheck)
.github/workflows/release.yml (1)
138-140: Verify permission preservation in artifact handling, sensei! 🔍
Let's ensure the executable permissions are preserved when the binaries are:
- Uploaded as artifacts
- Downloaded for Docker builds
- Copied into the Docker image
✅ Verification successful
Ohayo! The permission handling looks secure, sensei! ✨
The workflow correctly preserves executable permissions through the entire pipeline:
- Files are
chmod +xafter moving to platform directories - Files are preserved during artifact upload/download (actions/upload-artifact preserves permissions)
- Docker's COPY instruction preserves executable permissions by default when copying from artifacts to
/usr/local/bin/
The binaries will be executable in the final container as they are properly placed in /usr/local/bin/ with permissions intact.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the Dockerfile handles permissions correctly
# Test: Look for COPY commands and any explicit chmod operations
rg -A 2 'COPY.*\.(exe|katana|sozo|torii)' Dockerfile
Length of output: 54
Script:
#!/bin/bash
# Let's search for Dockerfile in the repository first
fd Dockerfile
# Then let's check the workflow file for artifact handling
cat .github/workflows/release.yml
Length of output: 7600
Script:
#!/bin/bash
# Let's check the Dockerfile content to see how artifacts are handled
cat Dockerfile
# Also check if there are any other Docker-related files
fd -e dockerfile -e Dockerfile
Length of output: 1028
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2604 +/- ##
==========================================
- Coverage 55.12% 55.12% -0.01%
==========================================
Files 385 385
Lines 47775 47775
==========================================
- Hits 26337 26335 -2
- Misses 21438 21440 +2 ☔ View full report in Codecov by Sentry. |
steebchen
force-pushed
the
perf/docker
branch
from
370c4ee
to
9d2b08f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
.github/workflows/preview.yml (2)
69-69: Ohayo! Consider removing debug command.The
ls -lahRcommand is helpful during development but might clutter the workflow logs in production.- ls -lahR artifacts
67-74: Ohayo sensei! The permission handling looks good.While permissions could theoretically be set during the Docker build or copy phase, having an explicit chmod step is more reliable and transparent. It ensures consistent permissions regardless of the host system's umask or Docker's copy behavior.
Alternative approaches could include:
- Setting permissions in the Dockerfile (less reliable due to build context)
- Using
COPY --chmodin the Dockerfile (requires BuildKit)- Setting permissions during the container copy (complex shell command)
The current approach is preferred for its simplicity and reliability.
🧰 Tools
🪛 actionlint
68-68: shellcheck reported issue in this script: SC2086:info:2:20: Double quote to prevent globbing and word splitting
(shellcheck)
68-68: shellcheck reported issue in this script: SC2086:info:3:20: Double quote to prevent globbing and word splitting
(shellcheck)
68-68: shellcheck reported issue in this script: SC2086:info:4:20: Double quote to prevent globbing and word splitting
(shellcheck)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
.dockerignore(1 hunks).github/workflows/preview.yml(1 hunks).github/workflows/release.yml(1 hunks)Dockerfile(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .dockerignore
- Dockerfile
🧰 Additional context used
🪛 actionlint
.github/workflows/preview.yml
68-68: shellcheck reported issue in this script: SC2086:info:2:20: Double quote to prevent globbing and word splitting
(shellcheck)
68-68: shellcheck reported issue in this script: SC2086:info:3:20: Double quote to prevent globbing and word splitting
(shellcheck)
68-68: shellcheck reported issue in this script: SC2086:info:4:20: Double quote to prevent globbing and word splitting
(shellcheck)
🔇 Additional comments (1)
.github/workflows/release.yml (1)
Line range hint 134-141: Verify permission handling across all platforms.
The current permission setting is only applied for Linux binaries. Let's verify if this is sufficient for the multi-platform Docker build.
✅ Verification successful
Ohayo sensei! After analyzing the chmod operations across the workflows, I can now provide a conclusive response.
Permission handling is properly implemented across platforms
The chmod operations are correctly applied in all relevant workflows:
- In
release.yml: Executables (katana, sozo, torii) are made executable during artifact creation - In
preview.yml: Same executables are properly chmod'ed for the linux/amd64 platform - In
ci.yml: Permissions are set appropriately for test and build environments
The current permission handling in the release workflow is sufficient because:
- Windows executables (.exe) don't require chmod operations
- The chmod operations are part of platform-specific jobs that run on the appropriate OS (matrix.job.os)
- The Docker build process preserves the executable permissions set during the artifact creation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if permissions are handled consistently across platforms
# and if the Dockerfile handles permissions correctly
# Test 1: Check if the Dockerfile uses COPY with mode preservation
echo "Checking Dockerfile for permission handling..."
rg -l 'COPY.*chmod|COPY.*--chmod' Dockerfile
# Test 2: Check if there are any other chmod operations in workflows
echo "Checking for other chmod operations in workflows..."
rg -l 'chmod' .github/workflows/
Length of output: 380
Script:
#!/bin/bash
# Let's check the specific chmod operations in these workflow files
echo "=== Checking chmod operations in release.yml ==="
rg -A 2 -B 2 'chmod' .github/workflows/release.yml
echo -e "\n=== Checking chmod operations in ci.yml ==="
rg -A 2 -B 2 'chmod' .github/workflows/ci.yml
echo -e "\n=== Checking chmod operations in preview.yml ==="
rg -A 2 -B 2 'chmod' .github/workflows/preview.yml
# Let's also check if there are any platform-specific conditions around these chmod operations
echo -e "\n=== Checking for platform-specific conditions ==="
rg -A 5 -B 5 'runs-on:|if:.*windows|if:.*macos|if:.*ubuntu' .github/workflows/release.yml
Length of output: 3696
steebchen
force-pushed
the
perf/docker
branch
from
9d2b08f
to
929e47f
Compare
Description
Consolidated chmod operations into the CI workflows and simplified Dockerfile COPY command for binaries.
Related issue
Tests
Added to documentation?
Checklist
scripts/prettier.sh,scripts/rust_fmt.sh,scripts/cairo_fmt.sh)scripts/clippy.sh,scripts/docs.sh)Summary by CodeRabbit