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

add dockerfile, update readme #1154

Closed
wants to merge 3 commits into from

Conversation

realies
Copy link

@realies realies commented Dec 3, 2023

could be used to fix #1138 if the image is published in a container image repository

@realies realies requested a review from lllyasviel as a code owner December 3, 2023 17:31
@superpoussin22
Copy link

running as root is a bad idea
check this : Dockerfile

@realies
Copy link
Author

realies commented Dec 4, 2023

running as root is a bad idea check this : Dockerfile

what's bad about it?
ps: why do you install the extra dependencies?

@superpoussin22
Copy link

as best practice it recommended to start process in a container as standard user and not as root

@Hummenix
Copy link

Hummenix commented Dec 7, 2023

@realies

what's bad about it?

Quite simply the same reason you shouldn't run any process as root if not absolutely necessary. It is possible to break out of a docker container (look up "docker privilege escalation/breakout" for more info), or it could be misconfigured and misbehave leading to issues with the host system.
Yes, docker is pretty well isolated, but the risk is still there, and there is no benefit from running as root. It's just a secure practice.

@realies
Copy link
Author

realies commented Dec 7, 2023

I've reduced the privileges in the latest commit.
@superpoussin22, wondering why your Dockerfile installs more dependencies.

Copy link

@matiboux matiboux left a comment

Choose a reason for hiding this comment

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

As a best practice, RUN install instructions in a Dockerfile should not leave cache behind. This helps to minimize the layers & image size. I suggested changes to clear apt's cache and disable pip3's cache.

Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
@realies
Copy link
Author

realies commented Dec 11, 2023

Thanks, @matiboux. I've applied your suggestions in the latest commit.

@whitehara
Copy link
Contributor

I also made docker related files and send PR as #1418
Shoud I merge this PR?

@realies
Copy link
Author

realies commented Dec 16, 2023

I also made docker related files and send PR as #1418
Shoud I merge this PR?

Dunno man, all other Dockerfiles I've seen so far look more complex without any (obvious to me) reason.

@whitehara
Copy link
Contributor

I also made docker related files and send PR as #1418
Shoud I merge this PR?

Dunno man, all other Dockerfiles I've seen so far look more complex without any (obvious to me) reason.

OK. Your Dockerfile is almost the same as mine. But my PR has other feature about environment variables.
So I think it depends on which is preferred by the repo owner to merge.

@mashb1t
Copy link
Collaborator

mashb1t commented Dec 30, 2023

#1418 is now more mature than this PR, please consider contributing your ideas there.

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.

add docker image
6 participants