fix: handle special cases for layer sync #4651
Conversation
| """ | ||
|
|
||
| def gather_resources(self) -> None: | ||
| zip_file_path = os.path.join(tempfile.gettempdir(), f"data-{uuid.uuid4().hex}") |
There was a problem hiding this comment.
nit: Should we try use Pathlib where we can instead of os.path?
There was a problem hiding this comment.
We could, but I just wanted to follow what we already have in this file. We could do it in another PR for refactoring purposes.
| @@ -591,7 +591,7 @@ def _collect_single_function_and_dependent_layers( | |||
| return | |||
|
|
|||
| resource_collector.add_function(function) | |||
| resource_collector.add_layers([l for l in function.layers if l.build_method is not None and not l.skip_build]) | |||
| resource_collector.add_layers([l for l in function.layers if BuildContext.is_layer_buildable(l)]) | |||
There was a problem hiding this comment.
Interesting. I wonder if there was a specific reason for this. Also looks like the function part doesn't filter. I assume that is likely a bug? If so, just want to note it for us to fix later (outside this pr).
There was a problem hiding this comment.
I am not sure. Existing logic was pretty same with is_layer_buildable. Only difference is, is_layer_buildable additionally checks if codeuri of the layer points to a ZIP file or not.
For function collection part, even if function is set SkipBuild: True, then running sam build {logical-id} will build the function. But if you run sam build to build entire stack, it will skip that function to build. So it feels like this is not the expected behavior.
| """ | ||
|
|
||
| def gather_resources(self) -> None: | ||
| self._zip_file = os.path.join(tempfile.gettempdir(), f"data-{uuid.uuid4().hex}") |
There was a problem hiding this comment.
How are we cleaning up these temp dirs for this?
There was a problem hiding this comment.
It is cleaned up here in parent class: https://github.com/aws/aws-sam-cli/blob/develop/samcli/lib/sync/flows/layer_sync_flow.py#L98
|
|
||
| if is_zip_file(layer.codeuri): | ||
| LOG.debug("Creating LayerSyncFlowSkipBuildZipFile for '%s' resource", resource_identifier) | ||
| return LayerSyncFlowSkipBuildZipFile( |
There was a problem hiding this comment.
Nit: This is how I read the name, which led to some confusion on my end. LayerSyncFlow + SkipBuild + Zip + File. Reading it this way, made me think we were zipping the file/folder but doesn't seem like that is the ended thing. Maybe we should rename or add in some "_" to signal to the reader the right thing from the name?
There was a problem hiding this comment.
Naming is hard and I am also not very happy with these names, open for any suggestion.
Which issue(s) does this change fix?
#4304
Why is this change necessary?
If a layer resource doesn't have
BuildMethodor marked withSkipBuild: True, SAM CLI should skip build step and directly zip contents of the target folder.How does it address the issue?
By checking layer details and assigning different sync flows depending on the layer details.
What side effects does this change have?
N/A
Mandatory Checklist
PRs will only be reviewed after checklist is complete
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.