-
Notifications
You must be signed in to change notification settings - Fork 89
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
use pipenv in image building dockerfile #36
Conversation
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
Is this ready for review? I ask because of the TODO? If not please put wip in the title. |
images/Dockerfile
Outdated
@@ -92,13 +92,16 @@ RUN cd /tmp && \ | |||
mv ks_0.8.0_linux_amd64/ks /usr/local/bin && \ | |||
chmod a+x /usr/local/bin/ks | |||
|
|||
COPY ./requirements.txt /tmp/requirements.txt | |||
COPY ./Pipfile ./Pipfile.lock /tmp/ |
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.
Does this get used anywhere?
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.
@jlewi can you try to toggle and enable https://travis-ci.org/kubeflow/testing? |
@jimexist Sorry missed your first ping. Travis should be enabled now. |
@jimexist Can you create instructions for updating/generating the Pipfile. |
@jlewi will do. by the way any way to update the cla flag? |
nevermind - i squashed locally |
@jlewi seems CLA bot didn't re-check. Can you help manually merge? |
this is cherry-picked from #36 so that it can unblock others.
@jimexist Can you update the Makefile so that you can build/push an image without tagging it latest? And then add a separate build command to tag it latest? Then can you build/push an image to kubeflow-ci. |
/assign jlewi |
@jimexist Were you able to test this image? If you wanted to you could create an E2E test (an Argo workflow) that would build the image and then do a smoke test on that image just by running a simple python script that would try to import various dependencies. |
Any update? |
… process. (kubeflow#47) We want to use a different image for security reasons because test code running in the test project could potentially modify the test worker image. To deal with kubeflow#46 remove hashes from requirements.txt
* Replace sha256sum because it doesn't work on OSX
/retest |
Thanks for updating it. Can you update the Makefile so I can try building an image and verify it works? |
@jimexist the Makefile was most likely updated in another file. |
@jlewi updated, could you take a look? |
py/kubeflow/testing/util.py
Outdated
@@ -47,7 +46,7 @@ def run(command, cwd=None, env=None, polling_interval=datetime.timedelta(seconds | |||
lines.append("{0}={1}".format(k, env[k])) | |||
logging.info("Running: Environment:\n%s", "\n".join(lines)) | |||
|
|||
log_file = None | |||
# log_file = None |
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.
Can you just delete this line?
Can you do
IIRC in a previous attempt there was a problem with pipenv not actually installing the packages so it would be good to verify its working now. |
|
|
|
|
@jlewi please take a look thanks |
Fantastic thank you so much. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jlewi The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Merging manually because of the CLA check. |
ready for review now