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 Dockerfile for Python 3 etc (#1047) #1055

Merged
merged 2 commits into from
Sep 15, 2017

Conversation

tripleee
Copy link
Member

@tripleee tripleee commented Sep 1, 2017

Update Dockerfile to use Python3 and correctly pull in dependencies. (#1047)
Also, create a separate SmokeDetector user instead of running as root.

  • Remove obsolete MAINTAINER direcive
  • Ubuntu provides python3-pip so no need to separately wget pip
  • Install SmokeDetector with a virtualenv so that tld can be updated by SmokeDetector
  • The image starts up with su - SmokeDetector by default
  • Optimize by using a single RUN directive

Update Dockerfile to use Python3 and correctly pull in dependencies.
Also, create a separate SmokeDetector user instead of running as root.

* Remove obsolete MAINTAINER direcive
* Ubuntu provides python3-pip so no need to separately wget pip
* Install SmokeDetector with a virtualenv so that tld can be updated by SmokeDetector
* The image starts up with su - SmokeDetector by default
Dockerfile Outdated
adduser --disabled-password --force-badname SmokeDetector \
--gecos SmokeDetector && \
su - SmokeDetector sh -c '\
env GIT_SSL_NO_VERIFY=true git clone https://github.com/Charcoal-SE/SmokeDetector.git && \
Copy link
Contributor

Choose a reason for hiding this comment

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

Why GIT_NO_SSL_VERIFY?

Copy link
Member Author

Choose a reason for hiding this comment

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

I just copy/pasted it from the old Dockerfile but you get weird errors from git if you don't have the full certificate chain for SSL verification. I'd imagine installing the ca-certificates package would fix that more properly. Thanks for inquiring; I'll see if I can get rid of that.

Copy link
Member

Choose a reason for hiding this comment

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

@tripleee Any updates on that? Because I'd rather use valid SSL, not invalid SSL. (ca-certificates is important after all to have so Smokey when it runs doesn't blow up on bad SSL)

Copy link
Member

@teward teward left a comment

Choose a reason for hiding this comment

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

Try and get ca-certificates installed so we don't need to use the SSL_NO_VERIFY, and instead verify valid SSL.

Dockerfile Outdated
adduser --disabled-password --force-badname SmokeDetector \
--gecos SmokeDetector && \
su - SmokeDetector sh -c '\
env GIT_SSL_NO_VERIFY=true git clone https://github.com/Charcoal-SE/SmokeDetector.git && \
Copy link
Member

Choose a reason for hiding this comment

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

@tripleee Any updates on that? Because I'd rather use valid SSL, not invalid SSL. (ca-certificates is important after all to have so Smokey when it runs doesn't blow up on bad SSL)

And remove the now obsolete GIT_SSL_NO_VERIFY=true from git clone
@tripleee
Copy link
Member Author

Sorry for dropping the ball on this. I have now updated the Dockerfile and pushed the resulting image to https://hub.docker.com/r/tripleee/smokedetector/

The resulting image is still quite large. I'd be tempted to refactor it to use Alpine instead of Ubuntu but that's a separate effort, and also depends on what the image is supposed to be used for.

@codecov-io
Copy link

codecov-io commented Sep 11, 2017

Codecov Report

Merging #1055 into master will increase coverage by 0.04%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1055      +/-   ##
==========================================
+ Coverage    63.8%   63.84%   +0.04%     
==========================================
  Files           6        6              
  Lines        1768     1770       +2     
==========================================
+ Hits         1128     1130       +2     
  Misses        640      640
Impacted Files Coverage Δ
findspam.py 82.58% <0%> (ø) ⬆️
globalvars.py 93.33% <0%> (+0.11%) ⬆️

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 f13f0af...a014cb7. Read the comment docs.

@tripleee
Copy link
Member Author

I struggled to find out how to mark the requested change as implemented. I guess the protocol is for @teward to approve the change.

@teward teward merged commit 8d1084d into Charcoal-SE:master Sep 15, 2017
@tripleee tripleee deleted the dockerfile-fix branch September 22, 2017 07:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants