Skip to content
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

chore: Optimized the API Dockerfile for better build time. #527

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

kash2104
Copy link

@kash2104 kash2104 commented Nov 10, 2024

User description

Fixes #176

Description

  • Efficient Caching: Optimized order of operations to leverage Docker’s build cache more effectively.
  • Removed Unnecessary Dependency: Retained only the necessary dependencies required for the /api service, removing any unnecessary packages that were previously included.

Fixes #[176]

Dependencies

N/A

Future Improvements

N/A

Mentions

@rajdip-b

Screenshots of relevant screens

N/A

Developer's checklist

  • My PR follows the style guidelines of this project
  • I have performed a self-check on my work

If changes are made in the code:

  • I have followed the coding guidelines
  • My changes in code generate no new warnings
  • My changes are breaking another fix/feature of the project
  • I have added test cases to show that my feature works
  • I have added relevant screenshots in my PR
  • There are no UI/UX issues

Documentation Update

  • This PR requires an update to the documentation at docs.keyshade.xyz
  • I have made the necessary updates to the documentation, or no documentation changes are required.

PR Type

enhancement


Description

  • Reordered the COPY commands to improve Docker caching efficiency, which can lead to faster build times.
  • Set NODE_ENV to production to ensure the production environment is correctly configured.
  • Adjusted file ownership and permissions to run the production container as a non-root user, enhancing security.
  • Removed the USER node command in the build stage as it was unnecessary.

Changes walkthrough 📝

Relevant files
Enhancement
Dockerfile
Optimize Dockerfile for improved build performance and security

apps/api/Dockerfile

  • Reordered COPY commands for better caching.
  • Changed NODE_ENV to production for production builds.
  • Adjusted file ownership and permissions for non-root user.
  • Removed unnecessary USER node command in the build stage.
  • +11/-9   

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    @kash2104 kash2104 requested a review from rajdip-b as a code owner November 10, 2024 16:55
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Build Order
    Verify that moving the package.json copy after pnpm install doesn't break dependency installation. The new order copies app-specific files after the install step.

    File Permissions
    Validate that changing file ownership from root:root to node:node in the production stage doesn't cause permission issues when running the application.

    Copy link
    Contributor

    codiumai-pr-agent-free bot commented Nov 10, 2024

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Enable container health monitoring through Docker's built-in health check mechanism

    Add HEALTHCHECK instruction to enable Docker to check container health status.

    apps/api/Dockerfile [47-49]

     EXPOSE ${API_PORT}
    +
    +HEALTHCHECK --interval=30s --timeout=3s CMD wget --no-verbose --tries=1 --spider http://localhost:${API_PORT}/health || exit 1
     
     ENTRYPOINT ["node", "/app/apps/api/dist/main.js"]
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Adding a HEALTHCHECK instruction is a valuable operational enhancement that would improve container monitoring and orchestration capabilities. This is particularly important for production environments, which this PR is configuring.

    8

    💡 Need additional feedback ? start a PR chat

    @rajdip-b
    Copy link
    Member

    @kash2104 remember to tag your pr when you create your pr.


    RUN --mount=type=cache,id=pnpm,target=/pnpm/store \
    pnpm install --ignore-scripts --frozen-lockfile && \
    rm -rf /root/.npm /root/.node-gyp /tmp/npm-*
    pnpm install --ignore-scripts --frozen-lockfile && \
    Copy link
    Member

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Could you use some tabs in here to ensure proper formatting?

    @rajdip-b rajdip-b changed the title perf: #176 Optimized the api Dockerfile for better build time. chore: Optimized the api Dockerfile for better build time. Nov 11, 2024
    @rajdip-b rajdip-b changed the title chore: Optimized the api Dockerfile for better build time. chore: Optimized the API Dockerfile for better build time. Nov 11, 2024
    Copy link
    Member

    @rajdip-b rajdip-b left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    • image
      Getting this error on running docker run --rm --network host --name ks-api --env-file .env ks-api
    • image
      Perhaps you could change the app location in the docker image to just be inside /app, and not /app/api

    @kash2104
    Copy link
    Author

    @rajdip-b I changed the app location to /app. Since this Dockerfile is for api, do we need the packages and root package.json?

    @rajdip-b
    Copy link
    Member

    The compiled version should have all the necessary dependencies. That would go into the 2nd stage. So i believe that you wont actually need to manually delete anything.

    @kash2104 kash2104 requested a review from rajdip-b November 15, 2024 16:01
    Copy link
    Member

    @rajdip-b rajdip-b left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    I'm still getting errors.
    image

    I would recommend you to try building this locally on your device using pnpm docker:build:api and then running the container using pnpm docker:run:api

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    Optimize API Dockerfile
    2 participants