-
-
Notifications
You must be signed in to change notification settings - Fork 698
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: Support installing poetry dependencies with pip #311
feat: Support installing poetry dependencies with pip #311
Conversation
examples/build-package/main.tf
Outdated
@@ -278,8 +302,31 @@ module "lambda_layer" { | |||
|
|||
build_in_docker = true | |||
runtime = "python3.8" | |||
docker_image = "public.ecr.aws/sam/build-python3.8" | |||
docker_image = "build-python3.8" |
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.
If public.ecr.aws/sam/build-python3.8
already exists locally, the docker_file
is not used.
pip_requirements_step( | ||
os.path.join(path, 'requirements.txt')) |
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.
pip_requirements_step()
already appends the filename if path
is a directory:
pip_requirements_step( | |
os.path.join(path, 'requirements.txt')) | |
pip_requirements_step(path) |
This PR has been automatically marked as stale because it has been open 30 days |
Still current |
This PR has been automatically marked as stale because it has been open 30 days |
Still current |
fdea8b9
to
c719460
Compare
I primarily use poetry with Python and appreciate this PR. I hope it is merged at some point in the future. Thanks for this work! |
@pdecat Please let us know if/when this PR is ready for review and merge? I expect that other users (including @1davidmichael @panessa @Tonkonozhenko) will be able to help with the review and testing. |
Hi, I believe it is ready for review and testing by others. |
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.
I added support for http-basic auth for poetry. Code was not taking in consideration poetry.toml file
64e76a4
to
37d2249
Compare
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.
Hi @panessa, thanks for the review! I've integrated your suggested changes, please take a look.
b7bdc36
to
bdbbb0f
Compare
Hi @antonbabenko, some Github Actions jobs failed because of network errors. Mind to restart them? 🙏 |
@pdecat Restarted GitHub Actions, and all checks are green. I have scheduled to revive this PR on the 13th of October (after HashiConf). Hopefully, everyone else will be addressing all the comments and do a review, too. |
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.
I just tried to test it locally after #362 was merged, and apparently, it broke package.py
you've created.
The error message I got during terraform destroy
after the successful run of rm -rf builds/lambda_layer/ && terraform apply -target module.lambda_layer
:
│ npm_requirements_step(
│ File ".../terraform-aws-modules/terraform-aws-lambda/examples/build-package/../../package.py", line 709, in npm_requirements_step
│ raise RuntimeError(
│ RuntimeError: Nodejs interpreter version equal to
│ defined lambda runtime (nodejs14.x) should be available
│ in system PATH
│
│ State: exit status 1
Could you please fix that and update README.md in the root to showcase poetry support in this section and maybe also in other places where it makes sense?
test_package_toml.py
Outdated
@@ -0,0 +1,10 @@ | |||
from package import get_build_system_from_pyproject_toml |
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 we move this file from the root to keep it clean?
Also, could you please update README with some instructions on how to run these? Or just 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.
Moved python unit tests to tests/
folder, add instructions to README.
And also added Github Action to run these tests against all supported python versions.
@@ -0,0 +1,2 @@ | |||
[build-system] |
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 file, too?
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.
Yes, it is needed by python unit tests. Moved it too.
fd8940d
to
09d78ca
Compare
09d78ca
to
472312c
Compare
The same errors happen using current master branch for all examples that do not have I see at least 3 solutions:
As a work-around for now, only destroy the modules that were applied with resource targeting:
|
…eses tests, and add instructions to README
36db3c8
to
1f960f6
Compare
The Pre-Commit / Max TF pre-commit check is failing with a weird error:
And does not only affect jobs from this PR, other PRs are also affected, see https://github.com/terraform-aws-modules/terraform-aws-lambda/actions/runs/3313158688/jobs/5470763712 Edit: this seems to be caused by terraform-linters/tflint-ruleset-terraform#42 |
1f960f6
to
876ffaa
Compare
Pre-commit hooks should be fixed now: https://github.com/terraform-linters/tflint/releases/tag/v0.42.2 |
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.
Works like a charm :) Thank you for your contribution!
## [4.3.0](v4.2.2...v4.3.0) (2022-10-31) ### Features * Support installing poetry dependencies with pip ([#311](#311)) ([398ae5a](398ae5a))
This PR is included in version 4.3.0 🎉 |
@pdecat @antonbabenko thank you for the work! we'll use it asap |
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
This issue has been resolved in version 4.16.0 🎉 |
Description
This PR adds support for installing poetry dependencies with
pip --no-deps
for projects withpyproject.toml
and optionallypoetry.lock
files.We use pip as as poetry does not currently provide a proper way to isolate installed dependencies from virtualenv (pip's
--target
option).--no-deps
disables pip's dependency resolver as poetry already pinned all dependencies.This is a continuation of #186
Motivation and Context
poetry's is more and more used and has a robust dependency resolver.
Breaking Changes
New poetry install step is not enabled by default and requires explicitly setting
poetry_install = True
in the source block.How Has This Been Tested?
examples/*
to demonstrate and validate my change(s)examples/*
projectspre-commit run -a
on my pull request