-
-
Notifications
You must be signed in to change notification settings - Fork 677
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 pip_tmp_dir
to allow setting the location of the pip temporary directory
#230
feat: Add pip_tmp_dir
to allow setting the location of the pip temporary directory
#230
Conversation
@antonbabenko any thoughts on this? |
@pdecat Could you please review this one (especially the Python part)? |
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, most changes relative to the addition of pip_tmp_dir
look correct to me, left a few remarks.
However, I'm still wondering about a concrete need for that.
@artificial-aidan, you mention "Can be useful for Docker in Docker builds.". Can you expand a bit why? In your actual use case, do you specify a special path that can be cached or something?
Also, wouldn't setting TMPDIR
, TEMP
or TMP
environment variable achieve the same?
README.md
Outdated
| <a name="provider_external"></a> [external](#provider\_external) | >= 1 | | ||
| <a name="provider_local"></a> [local](#provider\_local) | >= 1 | | ||
| <a name="provider_null"></a> [null](#provider\_null) | >= 2 | | ||
| <a name="provider_aws"></a> [aws](#provider\_aws) | 3.64.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.
I believe all these changes not related to the addition of pip_tmp_dir
should not be included in this PR.
@antonbabenko that's an issue I've always faced when running the terraform-docs pre-commit hook in this repository, but never asked about: what are we supposed to do about that?
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 was following the instructions on another PR. But I also don't know what to do with this one. It happened just when running the hooks.
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.
What seems especially wrong to me is replacing the flexible constraints coming from versions.tf
by hard-coded version numbers. I believe this is caused by the presence of .terraform.lock.hcl
files.
Probably caused by the fact that we are using too recent (>= 0.14) versions of terraform-docs that include terraform-docs/terraform-docs#509, while the Github Actions CI is using v0.13.0.
Note: terraform-docs v0.15 introduced a --lockfile
parameters that allows to disable that behavior: terraform-docs/terraform-docs#527
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.
Skip this issue, please. I know it was problematic for many for some time. I will fix the docs to make checks green if necessary after this PR is ready.
We are going to fix it in the near future like it is done here - terraform-aws-modules/terraform-aws-rds#361
runtime = "python3.8" | ||
source_path = [{ | ||
path = "${path.module}/../fixtures/python3.8-app1" | ||
pip_tmp_dir = "${path.cwd}/../fixtures" |
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.
Why path.cwd
instead of path.module
?
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.
Trying to remember now, but I believe that path.module
has a ../
prefix in it. Which something down the line did not like.
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.
path.module
seems to be a better choice here (see docs about path.cwd
).
Optionally, user can use something like - abspath(path.module)
if absolute path is required.
When using a remote Docker container for docker builds, the In theory those environment variables might work, (and would have saved me a lot of time probably) but now that I wrote it it seems like there could be a case where you didn't want all of the python temporary dirs to go to wherever you are setting your pip_temp_dir to be. |
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.
Just minor comments. Leave terraform-docs as the last step (I will fix).
runtime = "python3.8" | ||
source_path = [{ | ||
path = "${path.module}/../fixtures/python3.8-app1" | ||
pip_tmp_dir = "${path.cwd}/../fixtures" |
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.
path.module
seems to be a better choice here (see docs about path.cwd
).
Optionally, user can use something like - abspath(path.module)
if absolute path is required.
…rectory Signed-off-by: Aidan Jensen <aidan@artificial.com>
Signed-off-by: Aidan Jensen <aidan@artificial.com>
Signed-off-by: Aidan Jensen <aidan@artificial.com>
Signed-off-by: Aidan Jensen <aidan@artificial.com>
6cf4d2e
to
284f2ff
Compare
Wrapped up the minor comments and fixed a merge conflict. |
pip_tmp_dir
to allow setting the location of the pip temporary directory
# [2.28.0](v2.27.1...v2.28.0) (2021-12-10) ### Features * Add `pip_tmp_dir` to allow setting the location of the pip temporary directory ([#230](#230)) ([f5f86b5](f5f86b5))
This PR is included in version 2.28.0 🎉 |
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. |
…rectory
Signed-off-by: Aidan Jensen aidan@artificial.com
Description
Add the ability to set the location of the temporary directory for pip installs
Motivation and Context
When building in remote docker, you may need to set a shared location for the pip install directory
Breaking Changes
No it does not
How Has This Been Tested?
examples/*
projectsI added a test that uses a temp dir in the cwd, and watched that directory while it was used.