-
Notifications
You must be signed in to change notification settings - Fork 25
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
Docker Tags in line with ark-analysis Version #636
Conversation
@ngreenwald The readthedocs issue is being fixed in #650. The docker does get tagged with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some small stuff
.travis.yml
Outdated
- docker push "$TRAVIS_REPO_SLUG" | ||
- echo "$DOCKER_PASSWORD" | docker login -u "$DOCKER_USERNAME" --password-stdin | ||
- docker build -t "$TRAVIS_REPO_SLUG":"$TRAVIS_TAG" . 1> /dev/null | ||
# Tag the image with a version like v0.3.2, and create another tag for latest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment should be updated I think
Dockerfile
Outdated
@@ -5,6 +5,10 @@ RUN apt-get update | |||
|
|||
# install dependencies needed for setting up R | |||
RUN apt-get install -y lsb-release dirmngr gnupg apt-transport-https ca-certificates software-properties-common | |||
RUN apt-get install -y libharfbuzz-dev libfribidi-dev | |||
|
|||
# RUN apt-get -y build-dep libcurl4-gnutls-dev |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this here still?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I will get rid of that commented bit.
RUN R -e "install.packages('arrow')" | ||
RUN R -e "install.packages('data.table')" | ||
RUN R -e "install.packages('doParallel')" | ||
RUN R -e "install.packages('foreach')" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did moving the order of R vs python resolve the issues?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, however it's a quality of life change. The Docker builds the Dockerfile line by line, and we do not expect the R packages to be changed very often if at all. If Docker does not notice changes based on a line, then it'll skip it. However the Python stuff will always be changing, so it'll cut development build time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Can you put out a new release to make sure everything is working as expected?
If you haven't already, please read through our contributing guidelines before opening your PR
What is the purpose of this PR?
Closes #634, #659.
Currently
latest
always points to the most recent build of the docker image, which may not be the current version of the repository. As a best practice we should tag the docker image to the same version of ark-analysis.How did you implement your changes
Adjusted the docker build section of the
.travis.yml
to include the tag, and pushing it properly.Remaining issues
None atm.