-
Notifications
You must be signed in to change notification settings - Fork 160
fix: skip binary validation for copy-only workflows #913
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Changes from all commits
be690eb
22f1994
affd0ec
b1a4a62
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -67,8 +67,9 @@ def __init__(self, source_dir, artifacts_dir, scratch_dir, manifest_path, runtim | |
| if osutils is None: | ||
| osutils = OSUtils() | ||
| self.osutils = osutils | ||
| self._use_npm = osutils.file_exists(manifest_path) | ||
|
|
||
| if not osutils.file_exists(manifest_path): | ||
| if not self._use_npm: | ||
| LOG.warning("package.json file not found. Continuing the build without dependencies.") | ||
| self.actions = [CopySourceAction(source_dir, artifacts_dir, excludes=self.EXCLUDED_FILES)] | ||
| return | ||
|
|
@@ -210,8 +211,15 @@ def get_resolvers(self): | |
| """ | ||
| specialized path resolver that just returns the list of executable for the runtime on the path. | ||
| """ | ||
| if not self._use_npm: | ||
| return [] | ||
| return [PathResolver(runtime=self.runtime, binary="npm")] | ||
|
|
||
| def get_validators(self): | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [BUG] Returning an empty list from both
runtime_architectures = SUPPORTED_RUNTIMES.get(self.runtime, None)
if not runtime_architectures:
raise UnsupportedRuntimeError(runtime=self.runtime)
if self.architecture not in runtime_architectures:
raise UnsupportedArchitectureError(runtime=self.runtime, architecture=self.architecture)This is the only place in the package that enforces that check (
Since Skipping the binary lookup is the right goal; the runtime check should be preserved. One option is to keep the validator and run the runtime check explicitly rather than relying on the resolver loop, for example by validating in if not self._use_npm:
LOG.warning("package.json file not found. Continuing the build without dependencies.")
# still reject unsupported runtime/architecture combinations
RuntimeValidator(runtime=self.runtime, architecture=self.architecture).validate(runtime_path=None)
self.actions = [CopySourceAction(source_dir, artifacts_dir, excludes=self.EXCLUDED_FILES)]
returnNote that |
||
| if not self._use_npm: | ||
| return [] | ||
| return super().get_validators() | ||
|
|
||
| @staticmethod | ||
| def get_install_action( | ||
| source_dir: str, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[BUG] This branch changes behavior for
PythonUvWorkflow, which is outside the PR's stated scope of Node.js and Ruby, and can fail builds that previously succeeded.PythonUvWorkflowreturns[]from bothget_resolvers()andget_validators()unconditionally (aws_lambda_builders/workflows/python_uv/workflow.py:158-175), with the docstring stating that "UV doesn't need external validation of Python runtime paths." Before this change itsself.binarieswas{}, the loop body never executed, and no runtime/architecture check ran. After this change every workflow with empty binaries goes throughRuntimeValidator. The PR's own new tests confirm the shift —test_unsupported_runtime_is_rejectedandtest_unsupported_architecture_is_rejectedassert that uv builds now raiseWorkflowFailedError.Two concrete consequences:
RUNTIME_ARCHITECTURES(aws_lambda_builders/supported_runtimes.py) now hard-fails, even though uv can download that interpreter itself. That table has to be updated for each new Lambda Python runtime; uv builds previously did not depend on it.runtimeis documented as optional inLambdaBuilder.build(aws_lambda_builders/builder.py:62). A uv build invoked without a runtime now fails withRuntime None is not supportedwhere it previously proceeded. Node.js and Ruby are unaffected here, since both resolved a binary and ranRuntimeValidatorbefore this PR.Separately, hardcoding
RuntimeValidatorbypassesget_validators()entirely, so a workflow with a custom validator (e.g.GoRuntimeValidator,PythonRuntimeValidator) that adopts the same copy-only pattern would silently get base-class validation rather than its own.Consider making the opt-out explicit so the fallback only applies to workflows that want it, rather than to any workflow whose
binarieshappen to be empty:If applying runtime validation to uv builds is intentional, please call it out in the PR description and update the
PythonUvWorkflow.get_validators()docstring, which now contradicts the actual behavior.