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
Add small fix and test to assert prepare_host hook is called with correct dirs #8585
Conversation
@@ -403,6 +404,9 @@ def cmd_start(docker: bool, host: bool, no_banner: bool, detached: bool) -> None | |||
config.is_in_docker = False | |||
config.dirs = config.init_directories() | |||
|
|||
# call hooks to prepare host (note that this call should stay below the config overrides above) |
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.
This is effectively the main change in the PR - calling the hooks after we've adjusted the config
values above.
) | ||
|
||
# prepare config options | ||
image_name = ls_service_details.get("image") |
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: removed unused variable (duplicate from line 295 above, and not used further below)
@@ -153,7 +153,7 @@ def _worker(*_args): | |||
|
|||
def is_command_available(cmd: str) -> bool: | |||
try: | |||
run("which %s" % cmd, print_error=False) | |||
run(["which", cmd], print_error=False) |
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.
security: changing to the non-shell subprocess
execution here, to prevent potential command injection.
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.
nice catch. changes make sense to me!
Add small fix and test to assert that
prepare_host
hooks are called with appropriate LocalStack directories layout. This is a follow-up from #8384 .The previous PR #8384 adjusted the logic to start up LocalStack using the CLI when running inside a Docker container, for example to run
localstack start
in a Github Codespaces environment. This works well for the Community version, but unfortunately there was an oversight for the Pro version, where we're performing operations on the host viaprepare_host
hooks. In particular, the hook used to cache the key locally was failing with aPermissionError
, as it was trying to access the wrong directory (using the container dirs layout, not the host dirs layout).This PR contains a small follow-up change in
cmd_start(..)
to ensurebootstrap.prepare_host(..)
is called after we're performing theconfig.*
adjustments introduced in #8384 . The change has been tested in Github Codespaces, which now allows to spin up a Pro container via the CLI if the API key is configured. A unit test is added to cover the functionality, with assertions about the "is in Docker" status and the accessibility of thedirs.cache
directory.Note: The PR also contains a few minor cleanups/refactorings (add type hints, f-strings, etc) along the way.