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

feat: add pipfile support to docker image #808

Closed
wants to merge 2 commits into from

Conversation

lwywoo
Copy link
Contributor

@lwywoo lwywoo commented Oct 8, 2019

  • Ready for review
  • Follows CONTRIBUTING rules
  • Reviewed by Snyk internal team

What does this PR do?

Add pipfile support to dockerfile

Test Locally

docker build -t pipfiletest -f Dockerfile.python-3 . docker run -it -e SNYK_TOKEN=<INSERT_TOKEN> -v $PWD:/project pipfiletest:latest -d test

https://github.com/snyk/snyk/issues/786

@lwywoo lwywoo requested a review from a team as a code owner October 8, 2019 15:00
@ghost ghost requested review from gitphill and lili2311 October 8, 2019 15:00
source snyk/bin/activate
pip install -U -r "${PROJECT_PATH}/requirements.txt"
elif [ -f "${PROJECT_PATH}/Pipfile" ]; then
cd "${PROJECT_PATH}"
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this command would change the state of whatever gets executed afterwards. Not sure how to test this with confidence, maybe just install at the full path without cd?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Initially tried to install with full path but was getting:
✔ Successfully created virtual environment! Virtualenv location: /home/node/.local/share/virtualenvs/node-lHasYD0_ Creating a Pipfile for this project… Installing /project/Pipfile… WARNING: Invalid requirement, parse error at "'/project'" ✘ Installation Failed Failed to run the process ... Failed to test pip project

Will serarch the docs to see if I can find a more efficent way to do this

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks! Don’t think this is a ‘no go’, just worth seeing if we can keep the simplicity

Copy link
Contributor

Choose a reason for hiding this comment

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

To enter the directory, and then return from whence you came, you could replace this with:

pushd "${PROJECT_PATH}"

Then after running pipenv run popd

pip install -U -r "${PROJECT_PATH}/requirements.txt"
elif [ -f "${PROJECT_PATH}/Pipfile" ]; then
cd "${PROJECT_PATH}"
pipenv install Pipfile
Copy link
Contributor

Choose a reason for hiding this comment

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

Using pipenv install here has some interesting side effects that I don't think are desirable here. Consider the case you have a Pipfile and a Pipfile.lock.

My understanding is that pipenv install will install all dependencies, and update pipfile.lock with the versions it used. So it's not checking what you've checked in.

pipenv sync will install the exact versions specified in pipfile.lock and I think is a better solution when you detect a lock file.

The logic I have for this in the experimental images I'm plating with is https://github.com/garethr/snyk-images/blob/master/docker-entrypoint.sh#L14-L35

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the info, will update the PR :)

@dkontorovskyy
Copy link
Contributor

Why not use glob to match python custom requirements.txt files?

@miiila
Copy link
Contributor

miiila commented Nov 15, 2019

@dkontorovskyy Would you match all .txt files? Or just requirements-something?

@dkontorovskyy
Copy link
Contributor

All *req*.txt files

@miiila
Copy link
Contributor

miiila commented Nov 22, 2019

@dkontorovskyy I thought about it more and I'm probably for keeping it for requirements.txt only (or parametrize it somehow). Because we would need to check, if something matching req.txt and we would need find that particular name (I don't know, how to do it in shell), because we would need to provided the full name. Additionally - not sure what to do if there would be multiple req.txt files.

@garethr
Copy link
Contributor

garethr commented Nov 22, 2019

Note that the new, experimental, set of images supports these use cases out of the box.

https://github.com/snyk/snyk-images

  • It correctly detects Pipfile and can test Pipfile baseed projects
  • It supports an environment variable which can be used to override the defaults

More details in the README: https://github.com/snyk/snyk-images#running-bootstrap-commands

It may be worth trying that out.

@lili2311
Copy link
Contributor

Will add this support in this PR: #925 please come over to review here instead @garethr @adrukh

@darscan darscan deleted the feat/add-pipfile-to-dockerfile branch January 20, 2023 17:54
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.

6 participants