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

Support docker #14

Merged
merged 3 commits into from Sep 2, 2021
Merged

Support docker #14

merged 3 commits into from Sep 2, 2021

Conversation

AntiKnot
Copy link
Contributor

@AntiKnot AntiKnot commented Sep 2, 2021

Add Dockerfile, use gosu run specific application as specific user.

What problem does this PR solve?
Issue Number: close #13

What is changed and how it works?

  • Dockerfile
  • docker-entrypoint.sh
  • .dockerignore

@codecov-commenter
Copy link

codecov-commenter commented Sep 2, 2021

Codecov Report

Merging #14 (fd7aaf6) into master (ea8d923) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##            master       #14   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           16        17    +1     
  Lines          311       321   +10     
=========================================
+ Hits           311       321   +10     
Impacted Files Coverage Δ
rsserpent/models/__init__.py 100.00% <0.00%> (ø)
rsserpent/models/proxy.py 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ea8d923...fd7aaf6. Read the comment docs.

@queensferryme
Copy link
Member

Thanks for your contribution! Here are two of my concerns:

  • The .dockerignore seems incomplete (compared to .gitignore). For example the document build artifact site/ is not excluded from the docker image.
  • I don't quite understand gosu and docker-entrypoint.sh, could you provide some context or explanation?

@AntiKnot
Copy link
Contributor Author

AntiKnot commented Sep 2, 2021

Q1 .dockerignore
I didn't read the code completely, and the files that might be generated by the script were not removed.

Q2 gosu
Avoid using the root user to start the application, and can still handle users and groups after the image is built. gosu can intercept the command matched in cmd and execute it with the specified user instead.

ref.
repo https://github.com/tianon/gosu
demo repo https://github.com/sudo-bmitch/jenkins-docker

Thanks for your contribution! Here are two of my concerns:

  • The .dockerignore seems incomplete (compared to .gitignore). For example the document build artifact site/ is not excluded from the docker image.
  • I don't quite understand gosu and docker-entrypoint.sh, could you provide some context or explanation?

.dockerignore Outdated
dist/

# Docs
site/
Copy link
Contributor Author

Choose a reason for hiding this comment

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

update .dockerignore, add the content in .gitignore.

PTAL @queensferryme

@queensferryme
Copy link
Member

I made quite some changes to the PR:

  • use alpine python image, which reduce image size greatly;
    REPOSITORY            TAG          IMAGE ID       CREATED          SIZE
    rsserpent             latest       fea89a26b202   12 minutes ago   235MB
    rsserpent             0.1.3        eba50e4d545f   6 hours ago      1.07GB
    
  • use poetry install --no-dev, also reducing image size;
  • only copy pyproject.toml, poetry.lock, and rsserpent/, which helps keeping .dockerignore clean;
  • use COPY --chown syntax, removing docker-entrypoint.sh.

I am not so sure about the removal of docker-entrypoint.sh. In my understanding, your docker-entrypoint.sh uses gosu to change the permissions of workdir. To me COPY --chown seems like a better solution, since we don't need to introduce gosu and docker-entrypoint.sh.

I will merge if @EurusEurus are okay with these changes.

@AntiKnot
Copy link
Contributor Author

AntiKnot commented Sep 2, 2021

I made quite some changes to the PR:

  • use alpine python image, which reduce image size greatly;
    REPOSITORY            TAG          IMAGE ID       CREATED          SIZE
    rsserpent             latest       fea89a26b202   12 minutes ago   235MB
    rsserpent             0.1.3        eba50e4d545f   6 hours ago      1.07GB
    
  • use poetry install --no-dev, also reducing image size;
  • only copy pyproject.toml, poetry.lock, and rsserpent/, which helps keeping .dockerignore clean;
  • use COPY --chown syntax, removing docker-entrypoint.sh.

I am not so sure about the removal of docker-entrypoint.sh. In my understanding, your docker-entrypoint.sh uses gosu to change the permissions of workdir. To me COPY --chown seems like a better solution, since we don't need to introduce gosu and docker-entrypoint.sh.

I will merge if @EurusEurus are okay with these changes.

LGTM @queensferryme
ps.

$ docker-slim build rsserpent:5.3
rsserpent.slim           latest    1c0fe25629b7   18 seconds ago   115MB

https://dockersl.im/

@queensferryme
Copy link
Member

Thanks. I will use docker-slim in github actions.

@queensferryme queensferryme merged commit 8abac17 into RSSerpent:master Sep 2, 2021
queensferryme added a commit that referenced this pull request Sep 2, 2021
Co-authored-by: Queensferry <queensferry.me@gmail.com>
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.

Support Docker
3 participants