Fix: Pass a copy of environment variables when building layers - #3139
Conversation
| @skipIf(SKIP_DOCKER_TESTS, SKIP_DOCKER_MESSAGE) | ||
| def test_cached_build_with_env_vars(self): | ||
| """ | ||
| Build 2 times to verify that second time hits the cached build | ||
| """ | ||
| overrides = { | ||
| "FunctionCodeUri": "Python", | ||
| "Function1Handler": "main.first_function_handler", | ||
| "Function2Handler": "main.second_function_handler", | ||
| "FunctionRuntime": "python3.8", | ||
| } | ||
| cmdlist = self.get_command_list( | ||
| use_container=True, parameter_overrides=overrides, cached=True, container_env_var="FOO=BAR" | ||
| ) | ||
|
|
||
| LOG.info("Running Command (cache should be invalid): %s", cmdlist) | ||
| command_result = run_command(cmdlist, cwd=self.working_dir) | ||
| self.assertTrue( | ||
| "Cache is invalid, running build and copying resources to function build definition" | ||
| in command_result.stderr.decode("utf-8") | ||
| ) | ||
|
|
||
| LOG.info("Re-Running Command (valid cache should exist): %s", cmdlist) | ||
| command_result_with_cache = run_command(cmdlist, cwd=self.working_dir) | ||
|
|
||
| self.assertTrue( | ||
| "Valid cache found, copying previously built resources from function build definition" | ||
| in command_result_with_cache.stderr.decode("utf-8") | ||
| ) | ||
|
|
There was a problem hiding this comment.
I removed this test because TestRepeatedBuildHitsCache covers all this.
| template = "layers-functions-template.yaml" | ||
|
|
||
| @parameterized.expand([(True,), (False,)]) | ||
| @skipIf(SKIP_DOCKER_TESTS, SKIP_DOCKER_MESSAGE) |
There was a problem hiding this comment.
Can we just move this test under the "cache builds" class we already have?
There was a problem hiding this comment.
@jfuss You mean to move this test inside the TestBuildWithCacheBuilds class?
The problem is that TestBuildWithCacheBuilds uses dedup-functions-template.yaml, which doesn't have any Layers. So it won't test what we want.
Maybe a solution would be to refactor TestBuildWithCacheBuilds to use layers-functions-template.yaml, but I think dedup-functions-template.yaml fits better there. layers-functions-template.yaml has a lot of layers inside, so the test would get much slower without improving the coverage.
a84f5ee to
723e4fd
Compare
684a603 to
6ba0b68
Compare
mndeveci
left a comment
mndeveci
left a comment
There was a problem hiding this comment.
Thanks for the changes @Macok!
I verified your changes on my local machine and I can confirm that these changes are working as expected.
One thing though, when I tried to run integration test pytest tests/integration/buildcmd/test_build_cmd.py -k TestRepeatedBuildHitsCache it fails. Can you check what is the issue there?
@mndeveci If that's not it, could you post the logs? |
6ba0b68 to
66a392b
Compare
Thanks @Macok, I guess that was my bad at that time, when I run this I was able to verify that tests are working fine. |
|
@jfuss Only your approval missing. |
jfuss
added
stage/accepted
) (aws#3139) Co-authored-by: Mehmet Nuri Deveci <5735811+mndeveci@users.noreply.github.com> Co-authored-by: Sriram Madapusi Vasudevan <3770774+sriram-mv@users.noreply.github.com> Co-authored-by: Jacob Fuss <32497805+jfuss@users.noreply.github.com>
4 participants
Which issue(s) does this change fix?
#3133
Why is this change necessary?
During the build, additional env variable
LAMBDA_BUILDERS_LOG_LEVELis added to the dict.This additional variable ends up being persisted in build.toml, making cache always invalid.
This was already addressed here #2943 for Lambda Functions, but similar problems affects Lambda Layers.
How does it address the issue?
A copy of the dictionary is made, so the original dictionary stays unchanged.
What side effects does this change have?
None.
Checklist
make prpassesmake update-reproducible-reqsif dependencies were changedBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.