fix: Zip source directory should read from sh_work_dir #560
Conversation
ANGkeith
force-pushed
the
master
branch
2 times, most recently
from
9acecd0
to
bfa1bde
Compare
Signed-off-by: ANGkeith <ANGkeith@hotmail.sg>
ANGkeith
changed the title
ANGkeith
changed the title
There was a problem hiding this comment.
This looks good to me overall!
I'm just concerned it will cause issues for powershell users (I don't use that myself, so can't confirm). @antonbabenko is that actually a platform that is supposed to be supported by this module?
Apart from that, a few changes and it should be good IMO.
| ) | ||
| with tempfile.NamedTemporaryFile(mode="w+t", delete=True) as temp_file: | ||
| path, script = action[1:] | ||
| script = f"{script} && pwd >{temp_file.name}" |
There was a problem hiding this comment.
I guess this will break for powershell users, isn't it?
Maybe add a note that the pwd executed is required to determine the working dir of the subprocess shell after having executed all other commands. I find it a bit brittle, but now I understand that was a supported use case before #534.
There was a problem hiding this comment.
I guess this will break for powershell users, isn't it?
Not a powershell user too 😅 but hopefully this means we do not need to support pwsh
|
Thanks for the contribution and for the review. I agree, that this code will most likely break for Powershell users but I am still unsure if this module is even working for people who are not using Windows emulators like cygwin. Let's review this PR as for Mac/Linux usage, and fix/improve it if necessary for Windows later (unless someone wants to check it on Windows). |
|
We probably want to remove the powershell mention from this comment then: https://github.com/terraform-aws-modules/terraform-aws-lambda/pull/534/files#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5R442 |
## [7.2.6](v7.2.5...v7.2.6) (2024-04-12) ### Bug Fixes * Zip source directory should read from sh_work_dir ([#560](#560)) ([f786681](f786681))
|
This PR is included in version 7.2.6 🎉 |
|
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. |
fixes #557
Description
adding back the declaration of
sh_work_dirthat was removed in #534 because the variable is required and consumed hereAlso, had to refactor to write to a tempfile instead of using the fd
because in systems where
/bin/sh->/bin/dash, it would throw the following error