-
- Notifications
You must be signed in to change notification settings - Fork 739
feat: Support installing poetry dependencies with pip #311
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
Conversation
examples/build-package/main.tf Outdated
| 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-depsfor projects withpyproject.tomland optionallypoetry.lockfiles.We use pip as as poetry does not currently provide a proper way to isolate installed dependencies from virtualenv (pip's
--targetoption).--no-depsdisables 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 = Truein the source block.How Has This Been Tested?
examples/*to demonstrate and validate my change(s)examples/*projectspre-commit run -aon my pull request