feat: Add support for additional Docker options and pre pip/npm commands

[AllanBenson001]

Description

1. Have added support for additional options to be passed to the docker run command
2. Have added pre_package_commands to allow any number of commands to be run before running a pip or npm install
3. Have fixed where npm_requirements = claim.get('npm_package_json'), according to documentation and examples, this should be npm_requirements = claim.get('npm_requirements')
4. Have fixed issue introduced by fix: Generates error in plan phase if runtime is not available #358/fix: Performs plan-phase runtime check only if building package #359 raised in new runtime check fails for docker build #361

Motivation and Context

I want to build my lambdas in docker containers but I need access to packages in CodeArtifact. Differences in build environments make this slightly more difficult. On build machines builds are run within containers, so I have Docker in Docker issues. On developers machines, Terraform might be running within a container or it might not. Workarounds to cope with these different environments make for some very ugly code.

Therefore, easiest option is to make these changes that allow me to pass AWS tokens in as environment variables and run a CodeArtifact login within the container before doing a pip install. I see the need to be able to do things like pass tokens as environment variables or map local config files as volumes as being quite a common scenario for people wanting to get packages from private repositories (e.g. Artifactory).

I've made the changes very generic so they're flexible. Any option can be passed to docker run so it can set environment variables, volumes etc.

The pre package commands work both for docker and non-docker builds.

Breaking Changes

None.

How Has This Been Tested?

• I have updated at least one of the examples/* to demonstrate and validate my change(s)

• I have tested and validated these changes using one or more of the provided examples/* projects
  I've updated a couple of existing "build-package" tests and added a couple of new ones. In addition to that I've ran every test under examples/build-package

• I have executed pre-commit run -a on my pull request

[lorengordon]

I'm not sure about this. I think it is a pretty strong anti-pattern for a terraform plan to pass, that would fail in the apply phase. Every plan should expose issues with the pipeline, to catch issues as inputs change (e.g. the runtime was python3.8 but was updated to python3.9). If the system where I'm running the plan cannot actually build the package in the apply phase, that should be known before running the apply. That's true whether using docker or not.

However, I think we can make it flexible to the user. But instead of checking docker, check whether the user has explicitly told us to skip this check, using a new argument.

Suggested change

	if not query.docker and not shutil.which(runtime):
	if runtime_required and not shutil.which(runtime):

where runtime_required comes from the source_path map. E.g.

source_path = [
  {
    path             = "src"
    npm_package_json = true
    runtime_required = false
  }
]

[AllanBenson001]

Thanks for the review. After merging the latest changes into my branch, a lot of the build-package tests failed that were previously succeeding, this is because I don't have things like nodejs installed. The check above checks the runtime on the host machine, this is pretty pointless when you're building your package in a docker container (what would be the point in docker if you already had the correct runtime installed locally?). The change you've suggested would work, but I consider this to be a breaking change as anyone in the same situation as me would have to add that setting to get previously working scripts to succeed. If runtime_required defaulted to false (or rename to check_runtime or do_runtime_check that you have to set to true if you want to do the check), that would then not constitute a breaking change but it would almost render the runtime check useless so I'm not sure that's correct either (the runtime check is a good thing to have).

I fully agree that you don't want your plan to pass when the apply phase would fail, but neither do you want it to fail when the apply would have succeeded.

I'll await further feedback before proceeding.

[lorengordon]

I fully agree that you don't want your plan to pass when the apply phase would fail, but neither do you want it to fail when the apply would have succeeded.

The problem I see, is that relies on information not available to terraform at plan time. Only the user knows that the apply would succeed, because the user is proactively managing their pipeline separate from their local environment, and ensuring that the runtime is present. That feels... so very fragile. Change the runtime in the code, plan still claims success, merge, apply fails. Oh, update pipeline with new runtime, re-run job. That scenario is exactly what the test was meant to catch.

The check above checks the runtime on the host machine, this is pretty pointless when you're building your package in a docker container

Ok, that is news to me, and certainly a valid concern. I was under the impression this packaging script ran inside the docker container, where the runtime would be available, and of course the user should be specifying a container that has the correct runtime. That deserves some more investigation...

(Fwiw, I'm just another user/contributor, not a maintainer, but since I proposed the change that caused your breakage, I feel responsible for providing context around why the change was made, and trying to help find an acceptable way forward for both concerns.)

[AllanBenson001]

(Fwiw, I'm just another user/contributor, not a maintainer, but since I proposed the change that caused your breakage, I feel responsible for providing context around why the change was made, and trying to help find an acceptable way forward for both concerns.)

I appreciate your comments

Unless overridden, the runtime variable is used to choose the image that's used, see here, so it should be pretty safe.

I haven't contributed here before so I don't know if there's more that I need to do to get this PR approved. The change mentioned above is obviously mixed in with other changes which might not be approved, so it might be be necessary to fix this in a separate PR because as it stands, I'm not able to use the latest version of this module due to this error (and I've just seen that I'm not the only one - #361).

[lorengordon]

Unless overridden, the runtime variable is used to choose the image that's used, see here, so it should be pretty safe.

Ok. That does still run into problems when the user specifies image separately, and forgets to update the image when they update the runtime. But, at least it makes sense for the default case. And perhaps for now the relationship between a custom image and the runtime and plan vs apply-time failures can be covered in the README somewhere.

I do think it would be better to handle fixing the breakage in a separate PR. I can take a crack at that, or defer to you since the approach using query.docker is your idea?

[lorengordon]

I'll go ahead and cherry-pick your commit, and open the PR...

[lorengordon]

@antonbabenko @AllanBenson001 See #362

[antonbabenko]

@AllanBenson001 Thank you for the work you are doing on this PR!

It suddenly because too big :) If an issue was introduced recently (in #359), then let's fix it in isolation with the smallest PR possible.

Adding additional Docker options and pre-pip/npm commands is fine, but it should also be separated.

PS: With every change to the package.py it becomes harder to maintain this module. At some point, I want us to keep basic features related to the package building in this module and ask users to use docker-build submodule and deploy Docker images instead of packages. WDYT?

[AllanBenson001]

@AllanBenson001 Thank you for the work you are doing on this PR!

It suddenly because too big :) If an issue was introduced recently (in #359), then let's fix it in isolation with the smallest PR possible.

Adding additional Docker options and pre-pip/npm commands is fine, but it should also be separated.

PS: With every change to the package.py it becomes harder to maintain this module. At some point, I want us to keep basic features related to the package building in this module and ask users to use docker-build submodule and deploy Docker images instead of packages. WDYT?

I'm aware that you can deploy a lambda from a docker image, but that's not what I want to do. I know what you mean about package.py becoming hard to maintain, there are lots of options that make testing changes such as the ones @lorengordon made very difficult. The only thing I could suggest would be to separate it out into separate modules that create the zip file (could have separate modules for if you want to use docker or not) and then a single one to deploy the lambda, but that's obviously a lot of work and a lot of upheaval for anyone using this module.

If you want I can separate this out into separate PRs, the change to pass additional docker options contained far fewer changes the one to run those commands.

[AllanBenson001]

Closed and replaced by #366

[github-actions]

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.