-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
refactor: avoid unfavorable tar -> untar in container image #8439
Conversation
WalkthroughWalkthroughThe updates across various Dockerfiles and shell scripts primarily focus on streamlining operations by changing the way built code is handled—shifting from packaging into a tarball to direct copying. Additionally, there's an enhancement in the handling of environment variables and conditional checks, particularly around directory creation and AWS credentials. These changes aim to improve clarity and efficiency in the code deployment and execution process. Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration 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: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- packages/nocodb/Dockerfile (2 hunks)
- packages/nocodb/Dockerfile.local (2 hunks)
- packages/nocodb/docker/start-litestream.sh (2 hunks)
- packages/nocodb/docker/start-local.sh (1 hunks)
- packages/nocodb/docker/start.sh (1 hunks)
- packages/nocodb/litestream/Dockerfile (2 hunks)
Files skipped from review due to trivial changes (5)
- packages/nocodb/Dockerfile
- packages/nocodb/Dockerfile.local
- packages/nocodb/docker/start-local.sh
- packages/nocodb/docker/start.sh
- packages/nocodb/litestream/Dockerfile
Additional comments not posted (3)
packages/nocodb/docker/start-litestream.sh (3)
5-6
: LGTM! The directory creation logic is robust and correctly handles potential spaces in directory names.
Line range hint
9-18
: LGTM! The AWS credential checks are comprehensive, and the file handling logic is secure and robust.
18-23
: LGTM! The litestream commands are correctly quoted, and running the replication in the background is a good practice for performance optimization.
packages/nocodb/Dockerfile.local
Outdated
# Copy production code & main entry file | ||
COPY --from=builder /usr/src/appEntry/ /usr/src/appEntry/ |
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.
We have to copy build and node_modules
to /usr/src/app/
and L#31
can be removed.
Here we can add
COPY --from=builder /usr/src/app/ /usr/src/app/
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.
Ah sorry, I mixed up /usr/src/app/
and /usr/src/appEntry/
🙈. Should be fine now.
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: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- packages/nocodb/Dockerfile (2 hunks)
- packages/nocodb/Dockerfile.local (2 hunks)
- packages/nocodb/litestream/Dockerfile (2 hunks)
Files skipped from review as they are similar to previous changes (3)
- packages/nocodb/Dockerfile
- packages/nocodb/Dockerfile.local
- packages/nocodb/litestream/Dockerfile
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: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- packages/nocodb/Dockerfile (2 hunks)
- packages/nocodb/Dockerfile.local (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- packages/nocodb/Dockerfile
- packages/nocodb/Dockerfile.local
Change Summary
Before this change, the NocoDB app code was GZIP-compressed to a tar archive during container image generation and extracted again on the first run of the app. This is unfavorable for a containerized app since the extraction is repeated on every container restart (unless
/usr/src/appEntry
lives on persistent storage), which delays startup. Besides, adding a tar archive in the container image to be extracted at run-time actually increases the storage requirements for the app at run-time.This change removes the compression and decompression steps and instead directly copies the app code as-is. It is a prerequisite for #8347.
Additionally, some ShellCheck suggestions were applied to make the start scripts more robust / POSIX-compliant.
Change type
Test/ Verification
Untested.