Conversation
LocalStack integration with Pro 1 files - 2 1 suites - 2 1h 45m 59s ⏱️ + 10m 14s Results for commit 0496c40. ± Comparison against base commit 762339a. ♻️ This comment has been updated with latest results. |
alexrashed
left a comment
There was a problem hiding this comment.
The changes are looking good, I only added a minor nitpick. It's great to see this behavior being fixed. I'm really looking forward to extracting the CLI from the runtime and properly isolate the two.
But it seems there is a small thing missing: The pipeline is red because the LocalstackContainer class expects the config.dirs.tmp to be present for writing the logs (which is now is not anymore). As soon as this is fixed, we're good to go from my perspective!
localstack/cli/main.py
Outdated
| # indicate to the environment we are starting from the CLI | ||
| os.environ["LOCALSTACK_CLI"] = "1" |
There was a problem hiding this comment.
This module should only be loaded if the CLI is started, but maybe this should still be moved to the main function body to ensure that this cannot be set accidentally by importing this module?
|
good catch @alexrashed, pushed a fix for your comments |
b3002c8 to
0496c40
Compare
This PR adds a band-aid to a long-standing issue that
config.dirsis used when running CLI commands.This caused localstack to create the
.filesystemdirectory in the Python path, which was originally just designed for executing host mode in developer environments, even when simply using the CLI.Because the CLI and the runtime share control paths, there are situations where values from
config.dirsare used to create directories or files, even though they may not make sense in the context of the CLI.dirs.cacheis used by the analytics metadata (cache machine id file), and was used by theget_service_catalogcall (now removed)dirs.datais used bylocalstack_ext.config(this will be removed)dirs.logsis used as target to put the logs of the docker run command when starting localstackdirs.tmpshould be removed now, but left it in there just in caseThere is now a special control path that populates
config.dirsdeliberately with values that make the above examples work without the.filesystemdirectory.To reduce the number of calls to
config.dirs, I also removed some unused code from the startup procedure.Once the old edge proxy logic is removed, we can more easily split up
infra.pyand thereforebootstrap.py, which will hopefuly also make it easier to separate out the CLI code.