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

Update prod.Dockerfile #1041

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

adityakrmishra
Copy link

Explanation of Additions:
Environment Variables:

Set DEBIAN_FRONTEND=noninteractive to prevent interactive prompts during package installation.

Additional Tools:

Installed vim, net-tools, and iputils-ping for convenience and debugging purposes.

Healthcheck:

Added a health check to ensure that the application is running and healthy. The health check will periodically check the application's health endpoint.

These additions enhance the Dockerfile by providing useful tools and ensuring the health of the running container.

Explanation of Additions:
Environment Variables:

Set DEBIAN_FRONTEND=noninteractive to prevent interactive prompts during package installation.

Additional Tools:

Installed vim, net-tools, and iputils-ping for convenience and debugging purposes.

Healthcheck:

Added a health check to ensure that the application is running and healthy. The health check will periodically check the application's health endpoint.

These additions enhance the Dockerfile by providing useful tools and ensuring the health of the running container.
Copy link
Member

@debanjum debanjum left a comment

Choose a reason for hiding this comment

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

Hey @adityakrmishra, could you clarify why these changes are required? The code changes, descriptions in your PRs seem AI generated. See also #1042.

While its perfectly fine to get the help of AI coding assistants, you do need to be able to understand these changes well enough to justify it to others. Otherwise it creates an unreasonable burden on the developers reviewing these changes.

Comment on lines +23 to +26
# Install additional tools
vim \
net-tools \
iputils-ping && \
Copy link
Member

Choose a reason for hiding this comment

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

Less tools, less risk. Don't see the point of putting it in prod containers

Comment on lines -33 to +44
RUN sed -i "s/dynamic = \\[\"version\"\\]/version = \"$VERSION\"/" pyproject.toml && \
RUN sed -i "s/dynamic = \

\[\"version\"\\]

/version = \"$VERSION\"/" pyproject.toml && \
Copy link
Member

Choose a reason for hiding this comment

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

Don't see the point of this change

Comment on lines -62 to +76
ARG PORT
ARG PORT=8000
Copy link
Member

Choose a reason for hiding this comment

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

Don't see why PORT = 8000 was set?

Comment on lines +70 to +72
# Healthcheck
HEALTHCHECK --interval=30s --timeout=5s --start-period=30s --retries=3 CMD curl -f http://localhost:${PORT}/health || exit 1

Copy link
Member

Choose a reason for hiding this comment

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

We have external health checks, so will avoid using this for now

@adityakrmishra
Copy link
Author

Explanation of Additions to prod.Dockerfile:

Install Additional Tools:

vim, net-tools, and iputils-ping were installed for Comfort while developing and debugging. However, I realize now that it's a good practice to keep the production containers lean in order not to expose more risks. So, I'll remove these tools from the production Dockerfile for best practices.

Updating the Version in pyproject.toml:

I used sed to dynamically set the version in pyproject.toml based on the $VERSION environment variable. This was intended to manage versioning more flexibly. However, if version management is already handled via other methods like version control tags, I’ll review and possibly remove this to avoid unnecessary complexity.

Setting ARG PORT:

The ARG PORT=8000 was set as a default value, but I see that it's not needed since the port can be specified at runtime. I will remove this Unnecessary setting to streamline the Dockerfile.

Healthcheck:

The health check command was added to monitor the application's health by periodically checking the health endpoint. Given that external health checks are in place, I will remove this internal health check to avoid redundancy.

Conclusion:

I am thankful for the feedback and understand the importance of maintaining a clean and efficient production Dockerfile. Here are the steps I will take:

Remove additional debugging tools (vim, net-tools, iputils-ping).

Review and potentially remove the version changes in pyproject.toml.

Remove the Unnecessary ARG PORT=8000 assignment.

Remove the internal health check since external health checks are already in place.

if you accept my pr its a great contribution for me
Thank you for guidance. Please consider that my language communication skills are not very good, and on that reason , I used a ai to help explaining my prespective breifly and clearly. I hope that you can understand the Purposes and need of changes.

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

Successfully merging this pull request may close these issues.

2 participants