Add python packager - #7
Conversation
garnaat
left a comment
garnaat
left a comment
There was a problem hiding this comment.
A couple of nitpicks. Tests all run for me locally. Code is a bit complicated so I'll need more time later today to review fully but overall looks goo.
fb0256b to
cce6a7d
Compare
Port over the python dependency builder from Chalice. Also added a small wrapper class to adapt the interface to something more like what the Builder Action will use. The interface for the low level will stay the same, the wrapper PythonPipDependencyBuilder class can be modified as needed to match the expected interface as it evolves.
cce6a7d to
9ce42fc
Compare
sanathkr
left a comment
sanathkr
left a comment
There was a problem hiding this comment.
I did not review the actual packager logic because it is coming from Chalice. I reviewed only the integration pieces. My biggest comment is to use the scratch_dir provided by the workflow instead of the system's tempdir.
My intention was to get more control over where files get written. Do you think this approach makes sense in general? Am I trying to control things out of my scope?
| @@ -0,0 +1,19 @@ | |||
| # Enable container based builds | |||
There was a problem hiding this comment.
this file is already in develop. If you rebase, you should get it
| we can extend at a later date to add more options to how pip is | ||
| called. | ||
| """ | ||
| # TODO: The DependencyBuilder makes the assumption that it is running |
There was a problem hiding this comment.
How reliable is the process of finding a virtualenv matching the runtime we want? If it is a lot of guesswork, we can simply fail fast if python --version doesn't match the runtime we want.
If it is easy to discover virtualenvs, we can try to detect one and then fail if not found.
Either way, it is okay to make the assumption that users have a compatible version of Python/PIP available in their shell.
There was a problem hiding this comment.
Created #13 to investigate the virtualenv story
| packages is included in the error object's ``missing`` property. | ||
| """ | ||
| if self._has_at_least_one_package(requirements_filepath): | ||
| with self._osutils.tempdir() as tempdir: |
There was a problem hiding this comment.
Instead of the system defined tempdir, this must use the tempdir passed to the Workflow.
It allows the builder library to control where files get written within a workflow. It is an extra layer of protection to ensure workflows don't mess with random files in the filesystem.
| return self._osutils.joinpath(unpack_dir, contents[0]) | ||
|
|
||
| def get_package_name_and_version(self, sdist_path): | ||
| with self._osutils.tempdir() as tempdir: |
There was a problem hiding this comment.
use the workflow provided tempdir instead.
sanathkr
left a comment
sanathkr
left a comment
There was a problem hiding this comment.
Outstanding issues are minor. I created issues to track remaining work.
Port over the python dependency builder from Chalice. Also added a small wrapper class to adapt the interface to something more like what the Builder Action will use. The interface for the low level will stay the same, the wrapper PythonPipDependencyBuilder class can be modified as needed to match the expected interface as it evolves.
3 participants
Add low level python packager
Port over the python dependency builder from Chalice. Also added a small
wrapper class to adapt the interface to something more like what the
Builder Action will use. The interface for the low level will stay the
same, the wrapper PythonPipDependencyBuilder class can be modified as
needed to match the expected interface as it evolves.
I made a few other changes as well to get the tests running, make things importable, etc.