-
Notifications
You must be signed in to change notification settings - Fork 3
feat: add --no-reload flag to disable hot reload for local commands #56
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
Changes from all commits
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 |
|---|---|---|
|
|
@@ -102,6 +102,7 @@ def __init__( | |
| mount_symlinks: Optional[bool] = False, | ||
| no_mem_limit: Optional[bool] = False, | ||
| function_logical_ids: Optional[Tuple[str, ...]] = None, | ||
| no_reload: bool = False, | ||
| ) -> None: | ||
| """ | ||
| Initialize the context | ||
|
|
@@ -220,6 +221,7 @@ def __init__( | |
|
|
||
| self._mount_symlinks: Optional[bool] = mount_symlinks | ||
| self._no_mem_limit = no_mem_limit | ||
| self._no_reload = no_reload | ||
|
|
||
| # Note(xinhol): despite self._function_provider and self._stacks are initialized as None | ||
| # they will be assigned with a non-None value in __enter__() and | ||
|
|
@@ -256,7 +258,6 @@ def __enter__(self) -> "InvokeContext": | |
| ContainersMode.WARM: [self._stacks, self._parameter_overrides, self._global_parameter_overrides], | ||
| ContainersMode.COLD: [self._stacks], | ||
| } | ||
|
Author
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. 🟡 Unrelated whitespace change: This removes a blank line that was separating the dict definitions from the following comment block. Minor, but it's unrelated to the feature and adds noise to the diff. Consider reverting. |
||
|
|
||
| # don't resolve the code URI immediately if we passed in docker vol by passing True for use_raw_codeuri | ||
| # this way at the end the code URI will get resolved against the basedir option | ||
| if self._docker_volume_basedir: | ||
|
|
@@ -267,6 +268,9 @@ def __enter__(self) -> "InvokeContext": | |
| if self._function_logical_ids: | ||
| _function_providers_kwargs["function_logical_ids"] = self._function_logical_ids | ||
|
|
||
|
Author
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. 🟡 Consider warning when if self._no_reload and self._containers_mode != ContainersMode.WARM:
LOG.warning("--no-reload has no effect without --warm-containers") |
||
| if self._containers_mode == ContainersMode.WARM: | ||
| _function_providers_kwargs["no_reload"] = self._no_reload | ||
|
|
||
| self._function_provider = _function_providers_class[self._containers_mode]( | ||
| *_function_providers_args[self._containers_mode], **_function_providers_kwargs | ||
| ) | ||
|
|
@@ -573,6 +577,7 @@ def lambda_runtime(self) -> LambdaRuntime: | |
| image_builder, | ||
| mount_symlinks=self._mount_symlinks, | ||
| no_mem_limit=self._no_mem_limit, | ||
| no_reload=self._no_reload, | ||
| ), | ||
| ContainersMode.COLD: LambdaRuntime( | ||
| self._container_manager, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -264,6 +264,13 @@ def warm_containers_common_options(f): | |
| type=click.STRING, | ||
| multiple=False, | ||
| ), | ||
| click.option( | ||
| "--no-reload", | ||
| is_flag=True, | ||
|
Author
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. Consider: Should Since
At minimum, updating the help text to mention the dependency:
Author
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. 🟢 Nice help text — clearly explains the use case. Consider also mentioning that this flag only takes effect with |
||
| default=False, | ||
| help="Disable automatic reloading of Lambda functions when source code or templates change. " | ||
| "Useful on Windows where file watchers can cause significant latency with security software.", | ||
| ), | ||
| ] | ||
|
|
||
| # Reverse the list to maintain ordering of options in help text printed with --help | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -894,6 +894,7 @@ def __init__( | |
| use_raw_codeuri: bool = False, | ||
| ignore_code_extraction_warnings: bool = False, | ||
| function_logical_ids: Optional[Tuple[str, ...]] = None, | ||
| no_reload: bool = False, | ||
| ) -> None: | ||
| """ | ||
| Initialize the class with SAM template data. The SAM template passed to this provider is assumed | ||
|
|
@@ -933,7 +934,10 @@ def __init__( | |
| self.is_changed = False | ||
| self._observer = FileObserver(self._set_templates_changed) | ||
| self._observer.start() | ||
| self._watch_stack_templates(stacks) | ||
| if no_reload: | ||
|
Author
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: Observer is still started even when The self._observer = FileObserver(self._set_templates_changed)
self._observer.start()But the You should also skip if no_reload:
LOG.debug("Hot reload is disabled (--no-reload). Skipping template file observer.")
self._observer = None
else:
self._observer = FileObserver(self._set_templates_changed)
self._observer.start()
self._watch_stack_templates(stacks)
Author
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. 🔴 Observer still started when Consider wrapping both the observer creation and start in the condition: if no_reload:
LOG.debug("Hot reload is disabled (--no-reload). Skipping template file observer.")
self._observer = None
else:
self._observer = FileObserver(self._set_templates_changed)
self._observer.start()
self._watch_stack_templates(stacks)Note: you'd also need to guard |
||
| LOG.debug("Hot reload is disabled (--no-reload). Skipping template file observer.") | ||
| else: | ||
| self._watch_stack_templates(stacks) | ||
|
|
||
| @property | ||
| def stacks(self) -> List[Stack]: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -512,7 +512,7 @@ class WarmLambdaRuntime(LambdaRuntime): | |
| warm containers life cycle. | ||
| """ | ||
|
|
||
| def __init__(self, container_manager, image_builder, observer=None, mount_symlinks=False, no_mem_limit=False): | ||
| def __init__(self, container_manager, image_builder, observer=None, mount_symlinks=False, no_mem_limit=False, no_reload=False): | ||
|
Author
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. Nit: Line length exceeds 120 characters This line is quite long. Consider wrapping the parameters: def __init__(
self, container_manager, image_builder, observer=None,
mount_symlinks=False, no_mem_limit=False, no_reload=False,
):
Author
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. 🟡 Line too long: This line is ~113 characters after adding def __init__(
self, container_manager, image_builder, observer=None,
mount_symlinks=False, no_mem_limit=False, no_reload=False
): |
||
| """ | ||
| Initialize the Local Lambda runtime | ||
|
|
||
|
|
@@ -528,6 +528,7 @@ def __init__(self, container_manager, image_builder, observer=None, mount_symlin | |
| self._function_configs = {} | ||
| self._containers = {} | ||
| self._container_lock = threading.Lock() # Thread-safe container creation | ||
| self._no_reload = no_reload | ||
|
|
||
| self._observer = observer if observer else LambdaFunctionObserver(self._on_code_change) | ||
|
|
||
|
|
@@ -596,8 +597,11 @@ def create( | |
| return container | ||
|
|
||
| # Create new container | ||
| self._observer.watch(function_config) | ||
| self._observer.start() | ||
| if self._no_reload: | ||
| LOG.debug("Hot reload is disabled (--no-reload). Skipping file observer for '%s'", function_path) | ||
| else: | ||
| self._observer.watch(function_config) | ||
| self._observer.start() | ||
|
|
||
| container = super().create( | ||
| function_config, effective_debug_context, container_host, container_host_interface, extra_hosts | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2346,3 +2346,60 @@ def test_clean_runtime_containers_stops_emulator_container(self, log_mock): | |
| emulator_container.stop.assert_called_once() | ||
| log_mock.debug.assert_called_with("Stopping durable functions emulator container") | ||
| self.assertIsNone(runtime._durable_execution_emulator_container) | ||
|
|
||
|
|
||
| class TestWarmLambdaRuntime_no_reload(TestCase): | ||
| """Tests for --no-reload flag in WarmLambdaRuntime""" | ||
|
|
||
| def setUp(self): | ||
| self.manager_mock = Mock() | ||
| self.lambda_image_mock = Mock() | ||
| self.observer_mock = Mock() | ||
|
|
||
| @patch("samcli.local.lambdafn.runtime.LambdaContainer") | ||
| def test_no_reload_skips_observer_watch_and_start(self, LambdaContainerMock): | ||
| """When no_reload=True, observer.watch and observer.start should NOT be called""" | ||
| container = Mock() | ||
| LambdaContainerMock.return_value = container | ||
|
|
||
| runtime = WarmLambdaRuntime( | ||
| self.manager_mock, self.lambda_image_mock, observer=self.observer_mock, no_reload=True | ||
| ) | ||
| runtime._get_code_dir = Mock(return_value="/some/code/dir") | ||
|
|
||
| func_config = Mock() | ||
| func_config.full_path = "MyFunction" | ||
| func_config.packagetype = "Zip" | ||
| func_config.durable_config = None | ||
| func_config.env_vars.resolve.return_value = {} | ||
| func_config.layers = [] | ||
| func_config.runtime_management_config = None | ||
|
|
||
| runtime.create(func_config) | ||
|
|
||
| self.observer_mock.watch.assert_not_called() | ||
| self.observer_mock.start.assert_not_called() | ||
|
|
||
| @patch("samcli.local.lambdafn.runtime.LambdaContainer") | ||
| def test_reload_enabled_calls_observer_watch_and_start(self, LambdaContainerMock): | ||
| """When no_reload=False (default), observer.watch and observer.start should be called""" | ||
| container = Mock() | ||
| LambdaContainerMock.return_value = container | ||
|
|
||
| runtime = WarmLambdaRuntime( | ||
| self.manager_mock, self.lambda_image_mock, observer=self.observer_mock, no_reload=False | ||
| ) | ||
| runtime._get_code_dir = Mock(return_value="/some/code/dir") | ||
|
|
||
| func_config = Mock() | ||
| func_config.full_path = "MyFunction" | ||
| func_config.packagetype = "Zip" | ||
| func_config.durable_config = None | ||
| func_config.env_vars.resolve.return_value = {} | ||
| func_config.layers = [] | ||
| func_config.runtime_management_config = None | ||
|
|
||
| runtime.create(func_config) | ||
|
|
||
| self.observer_mock.watch.assert_called_once_with(func_config) | ||
| self.observer_mock.start.assert_called_once() | ||
|
Author
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. Suggestion: Assert the container is still created successfully when The tests verify observer behavior but don't assert that the container itself was still created and returned. Adding assertions like: result = runtime.create(func_config)
self.manager_mock.create.assert_called_once()
self.assertIsNotNone(result)would strengthen the test by verifying that skipping the observer doesn't break container creation. |
||
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.
Nit: Unrelated whitespace change
This removes a blank line that was separating two logical blocks. It's a minor formatting change but unrelated to the feature — I'd restore the original blank line to avoid noise in the diff.