Skip to content
This repository has been archived by the owner on Jun 8, 2022. It is now read-only.

Support custom build script #30

Closed
wants to merge 9 commits into from

Conversation

asavoy
Copy link
Contributor

@asavoy asavoy commented Jan 2, 2019

Adds a build_script variable to allow specifying a custom build script.

This is intended as an escape hatch for use cases where the default build script is not suitable. An example is provided in examples/build.sh.

I've opened a PR to get a sense of whether there is interest in this. If so, I'm open to changes and suggestions (perhaps more documentation and a simpler build example).

@raymondbutcher
Copy link
Contributor

Hi @asavoy. I like this, thanks for submitting it.

Could you please use the example to create a new test under the tests directory? That will serve as a full example and show usage, as well as allow me to try it out.

I think the references to Terraform External Data Sources and Terraform External Program Protocol in your example script are wrong because 1) it's base 64 encoded and 2) it's being passed in as an argument, neither of which are part of Terraform's specs. It's just how hash.py happens to be passing data into build.py.

@asavoy asavoy force-pushed the custom-build-script branch from 6a05cb8 to c4d0456 Compare January 4, 2019 08:02
@asavoy
Copy link
Contributor Author

asavoy commented Jan 4, 2019

@raymondbutcher

Thanks for the corrections, I've changed accordingly.
I've turned the example into a test. Let me know if you have any further feedback.

asavoy and others added 7 commits January 4, 2019 16:05
An escape hatch for use cases where the built-in build script is not
usable; one example is that the Python dependencies require
platform-dependent native extensions.
It supports `base64 -d` or `base64 --decode` but not `base64 -D`. Not sure where `-D` is supported but I'm hoping `--decode` works there.
When I tried this, it was creating the Lambda build files as the root user and then it failed trying to clean them up. This now does everything including setting ownership inside Docker.

I also removed the default values because I'd rather it break then default to something that may not be intended.
@raymondbutcher
Copy link
Contributor

I've made a branch https://github.com/claranet/terraform-aws-lambda/compare/ray/custom-build-script with a couple of tweaks and fixes. Please take a look and let me know what you think. I would have tried to push it to your branch (not sure if I have permissions or not) but it required rebasing to fix the merge conflicts and I didn't want to do a git push -f on someone else's branch :)

I like this feature. I am also thinking that separately we should add this Docker behaviour to build.py and have a flag like var.build_with_docker so that this fairly common use case can be handled without a custom build script (but still keep the option for using a custom build script).

@raymondbutcher
Copy link
Contributor

One thing we need to solve is that #31 changes hash.py from:

build_command = "{build_script} {build_data}".format(...)

to:

build_command = "python {build_script} {build_data}".format(...)

So it will use whichever version of Python is running, in a way that works in both Linux and Windows.

We don't want to run all custom build scripts with Python, so python could be moved into the build script argument. However, the build script argument is used when generating the hash. Do we need 2 arguments, build_script and build_command or build_interpreter? That doesn't seem elegant.

@asavoy asavoy force-pushed the custom-build-script branch from 51c1a43 to 26b50cb Compare January 6, 2019 09:18
@asavoy
Copy link
Contributor Author

asavoy commented Jan 6, 2019

@raymondbutcher Cool, thanks for looking into it so quickly! Appreciate the time you've taken to get it working under Linux.

My notes so far:

  • I've tested the new branch and it works correctly (I'm running off macOS)

  • I've reset my branch to match yours

  • base64 -D must be macOS-specific, but base64 --decode is fine

  • Regarding removing default args from the build script, I've updated the Usage section in the script accordingly. Also fixed a typo near there.

  • I think first-class support for Docker builds is a great idea. I think it is difficult to use this module without it, since so many useful Python packages need native extensions. Perhaps it might make a nice successor to this feature.

  • Regarding the change to build_command for Windows support. I agree that adding a parameter like build_interpreter isn't elegant. Are we sure that changing to python build.py is necessary? If Python is correctly installed, build.py will execute properly in Windows. At least that's the case when using the command prompt on my Windows machine.

@raymondbutcher raymondbutcher mentioned this pull request Jan 7, 2019
@raymondbutcher
Copy link
Contributor

I'm leaning towards:

module "build_with_single_shell_script" {
  ...
  build_command = "${path.module}/custom-build.sh '$filename' '$source' '$runtime'"
  build_paths   = ["${path.module}/custom-build.sh"]
  ...
}

module "build_with_python" {
  ...
  build_command = "python ${path.module}/custom-build/build.py --filename='$filename' --source='$source' --runtime='$runtime'"
  build_paths   = ["${path.module}/custom-build", "${path.module}/other-dependencies"]
  ...
}

module "build_with_make" {
  ...
  build_command = "make --directory=${path.module}/custom-build filename='$filename' source='$source' runtime='$runtime'"
  build_paths   = ["${path.module}/custom-build"]
  ...
}

Notes:

  • hash.py replaces $filename with the calculated filename, plus $source and $runtime. Your example script would no longer require base64 and jq as it could just use positional arguments.
  • it requires specifying the build source files/directories separate to the command (so the files can be hashed and lambda package rebuilt whenever the build scripts change) but I think this might be necessary if it's going to be flexible
  • it supports hashing files or directories if the build script uses multiple files

@raymondbutcher
Copy link
Contributor

Hi @asavoy I've submitted #34 which rebased your branch and fixed the conflicts, then implemented the build_command/build_paths changes I suggested here on top of that. Would you mind taking a look before I merge it?

@asavoy
Copy link
Contributor Author

asavoy commented Jan 30, 2019

Hi @raymondbutcher, unfortunately I don't expect to have time to review anything for at least a few weeks due to work commitments and vacation. Don't let me hold you back, but just wanted to clarify when I'll next be able to return to it. Thanks for your efforts in integrating the feature.

@raymondbutcher
Copy link
Contributor

No worries. I've merged the other one just now. Thanks again!

mbklein pushed a commit to nulib/terraform-aws-lambda that referenced this pull request Apr 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants