-
Notifications
You must be signed in to change notification settings - Fork 33
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
Added src/docker/Dockerfile #83
Conversation
@original-brownbear Who has to build the image and push it to dockerhub? I suppose Yegor, since rultor's image is also under his name. |
@amihaiemil yes @yegor256 needs to setup an automated build for this. |
@amihaiemil Many thanks, I will find someone to review it before we merge |
@mkordas could you please review this pull request |
@amihaiemil I'm on it |
#RUN /bin/bash -l -c "gem update" | ||
#RUN /bin/bash -l -c "gem install nokogiri" | ||
#RUN /bin/bash -l -c "gem install bundler" | ||
#RUN echo 'source /usr/share/rvm/scripts/rvm' >> /etc/bash.bashrc |
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.
@amihaiemil If it's commented out, why not delete it?
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.
@mkordas I removed commented block.
@amihaiemil please see inital review. I think we should begin with minimal image instead. |
@amihaiemil I'm back |
@original-brownbear I need hour advice here - should we clean the things up here or leave as it is? E.g. nodejs is clearly not needed, but @amihaiemil did what he was told to do - just copy the image. |
@@ -0,0 +1,97 @@ | |||
# Copyright (c) 2012-2015, jcabi.com |
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.
@amihaiemil now we need to upload this image to Docker Hub
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.
@amihaiemil and of course update .rultor.yml
to use it
@mkordas sure :) Damn looking at that ticket again it's worded in a little unfortunate manner :) But agreed, simply copying the image was my suggested quick fix, obviously it's not even close to being an optimal solution. In a sense it is my mistake not that of the performer that the description leads to a rather iffy solution now.
Makes sense ? :) |
@mkordas As the author suggested, I removed the apt-get upgrate/update commands and added puzzle for packages version lock. Yegor has to set automated build of the image on Dockerhub, not me. After he is done, I willl specify the image in |
@amihaiemil I will check it now |
@@ -95,6 +95,9 @@ | |||
* and verify the expected HTTP requests have been sent to the mocked server. | |||
* @todo #69:30min Fix NullPointerExceptions in CL build for Open JDK6. | |||
* @todo #69:30min Fix UnsupportedClassVersionError in CL build for Open JDK6. | |||
* @todo #82:30min Clean unnecessary docker setup (src/docler/Dockerfile) and set | |||
* versions to the used packages (e.g. lock java version instead of default | |||
* jdk). Discuss with architect. |
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.
@amihaiemil I don't think that much discussion is needed here. I would stress that the goal here is to have minimal docker image with all versions locked that is enough to build this project.
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.
@mkordas I meant discuss the needed versions. Because for sure, the first question will be "OK, but what versions exactly are needed?"
@amihaiemil so we need additional puzzle just for Yegor, right? How we are going to track these tasks? |
@amihaiemil please see my comments above |
@mkordas yes, Yegor needs to do this. |
@yegor256 please merge with a button as explained by @original-brownbear above |
@amihaiemil merged |
@yegor256 Thanks |
@original-brownbear @yegor256 maybe let's try to do the deployment with a new image |
@rultor deploy |
@mkordas yea that should work, otherwise we need to reopen the issue I'm afraid. |
@rultor deploy now pls |
@amihaiemil @dmarkov Oops, I failed. You can see the full log here (spent 24s)
|
@original-brownbear Do you happen to know what went wrong? It seems to me some config on Yegor's DockerHub? He needs to define the 'latest' tag? |
@amihaiemil yes exactly, |
@amihaiemil it is "defined", as far as I understand: If you need me to do something else, please give more detailed instructions |
@yegor256 The build for this thing failed ... maybe hit the trigger button there and see if it works on retry before the Dockerfile issue gets reopened ... ( maybe we get lucky and it was just a random apt issue, seems it didn't find some package from the Dockerhub logs). |
@original-brownbear triggered it a second ago... let's wait |
@yegor256 it cannot work ... The
one |
@original-brownbear fixed in 5f445b0 |
@yegor256 phew ... you fixed it in a different instance 2 occurrences left still: how on earth did we all miss that when reviewing this ... |
@original-brownbear fixed in 9b81a37 |
@yegor256 thanks! looks good now, build on Dockerhub worked, I guess we can try deploying again :) |
@original-brownbear cool! |
PR regarding issue #82 . As explained by @original-brownbear , first step is to add this file, then build and push the image to Dockerhub, and then specify that image in .rultor.yml .