Skip to content
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

restructure localstack container bootstrapping and add CLI commands #4969

Merged
merged 7 commits into from Nov 20, 2021

Conversation

@thrau
Copy link
Member

@thrau thrau commented Nov 19, 2021

This PR adds the following CLI commands and the necessary restructuring to make them possible:

  • localstack start --detached: starts the localstack container in the background
  • localstack wait blocks until localstack is running (the ready marker has been printed)
  • localstack logs --follow tails the log output of localstack

The most noteworthy changes and improvements:

  • The most fundamental change is that, in the start_infra_in_docker routine, the terminal output no longer comes directly from the docker run command itself, but is forwarded to a logfile in the TMP folder that is then tailed using a new uiltity called FileListener (that is platform independent, but needs to be tested on Windows). that log is then printed to stdout asynchronously, while waiting on the docker run command to stop (via the Server abstraction)
  • added the CLI commands
  • The part of start_infra_in_docker that was building the command is now separated into a holder object (LocalstackContainer) and a configuration routine (configure_container)
  • adds a dedicated test step for bootstrapping (which involves starting up and tearing down the container).
  • FORCE_NONINTERACTIVE is not used anymore (there's no obvious reason to have an interactive tty docker session when only logs are printed to stdout)

Some things that I hope will be useful in the future:

  • with the LocalstackContainerServer, you can now treat localstack as you would any Server implementation. this can be used in tests to start up and tear down localstack without mucking about with low-level docker commands.
  • the LocalstackContainer object can be passed to configuration hooks that modify the startup behavior, without being part of the actual startup routine
  • being able to start localstack in detached mode via the CLI paves the way for a new type of UX

Things that need improving:

  • more bootstrap tests! especially for testing custom docker flags and volume mounts
  • The way the logfile is resolved for the LocalstackContainer is sort of hacky
  • The CmdDockerClient is used to allow the --privileged flag for the container startup
  • extend the item types VolumeMappings and use that throughout the ContainerClient abstraction (some ideas in this gist: https://gist.github.com/thrau/b450e79c6a08d965ca2731b26065b67c)
  • the ContainerClient abstraction does not provide any way of capturing or streaming the output of commands to something outside of the abstraction. i added default_out_file as a workaround, which is handed down as outfile parameter to the underlying subprocess to be able to capture the output of a run_container execution
  • the CI build step starts up localstack containers with the latest image, not the one built in the pipeline. this is only an issue when there is new interaction between the cli and the container runtime.
@thrau thrau had a problem deploying to localstack-ext-tests Nov 19, 2021 Error
@thrau thrau requested review from dfangl and whummer and removed request for dfangl Nov 19, 2021
@thrau thrau had a problem deploying to localstack-ext-tests Nov 19, 2021 Failure
@github-actions
Copy link

@github-actions github-actions bot commented Nov 19, 2021

LocalStack integration with Pro

    1 files  ±0      1 suites  ±0   22m 32s ⏱️ +12s
684 tests ±0  657 ✔️ ±0  24 💤 ±0  3 ±0 

For more details on these failures, see this check.

Results for commit d7d073e. ± Comparison against base commit 5657356.

♻️ This comment has been updated with latest results.

Loading

Copy link
Member

@whummer whummer left a comment

Great set of changes @thrau ! 🎉 Only a few minor comments.. Nice to see the Server abstraction being used for the LS container as well now. 👍

Loading

localstack/utils/bootstrap.py Outdated Show resolved Hide resolved
Loading
localstack/utils/common.py Outdated Show resolved Hide resolved
Loading
localstack/utils/docker_utils.py Outdated Show resolved Hide resolved
Loading

result = runner.invoke(
cli, ["wait", "-t", "0.5"]
) # on day this test will hopefully fail ;-)
Copy link
Member

@whummer whummer Nov 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😄 💯

Loading

@thrau thrau had a problem deploying to localstack-ext-tests Nov 20, 2021 Error
@thrau thrau force-pushed the refactor-docker-startup branch from 54ce364 to d7d073e Nov 20, 2021
@thrau thrau had a problem deploying to localstack-ext-tests Nov 20, 2021 Failure
Copy link
Member

@whummer whummer left a comment

🚀

Loading

@coveralls
Copy link

@coveralls coveralls commented Nov 20, 2021

Coverage Status

Coverage increased (+0.3%) to 81.638% when pulling d7d073e on refactor-docker-startup into 5657356 on master.

Loading

@thrau thrau merged commit 304fa0c into master Nov 20, 2021
9 of 11 checks passed
Loading
@thrau thrau deleted the refactor-docker-startup branch Nov 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants