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

Fix CLI command to enable starting the proxy from within Docker #77

Merged
merged 4 commits into from
Jul 25, 2024

Conversation

whummer
Copy link
Member

@whummer whummer commented Jul 24, 2024

Update environment check to use SDK Docker client and enable starting the proxy from within Docker.

The main use case is the ability to run the proxy container from within the LocalStack main container as part of an init script. /cc @RobertLucian

@whummer whummer requested a review from RobertLucian July 24, 2024 16:22
@whummer whummer force-pushed the proxy-from-docker branch 3 times, most recently from 135669f to 1d5a079 Compare July 24, 2024 18:01
@whummer whummer force-pushed the proxy-from-docker branch from 1d5a079 to 1c0b8a1 Compare July 24, 2024 18:26
Copy link
Contributor

@RobertLucian RobertLucian left a comment

Choose a reason for hiding this comment

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

LGTM!

That being said, while we're at it, do you mind also adding rolo as a dependency to the localstack-extension-aws-replicator package, and locking its version to <=0.6? If not, I'll have a separate PR created after this.

aws-replicator/Makefile Show resolved Hide resolved
@RobertLucian RobertLucian added the bug Something isn't working label Jul 24, 2024
@whummer
Copy link
Member Author

whummer commented Jul 24, 2024

That being said, while we're at it, do you mind also adding rolo as a dependency to the localstack-extension-aws-replicator package, and locking its version to <=0.6? If not, I'll have a separate PR created after this.

@RobertLucian not sure this is required at this point. Things are a bit in flux right now, admittedly (couple of package renamings and refactorings recently), but with the upcoming LocalStack 3.6.0 release this week things should stabilize again. I would suggest to better be forward-compatible and ask customers to upgrade to the latest version, which should make the rolo version pin unnecessary. Thoughts?

@alexrashed
Copy link
Member

alexrashed commented Jul 25, 2024

Just chiming in here concerning the rolo pin:
We definitely need to find ways to define compatibilities between LS versions and extensions, but extensions are installed into a vent which is then directly linked to the LocalStack venv. This means that LocalStack and it's transitive dependencies (like rolo) are "provided" and should not be set as install dependencies for extensions.
Setting a limit on rolo could therefore break the extension for certain versions of LocalStack.

@whummer
Copy link
Member Author

whummer commented Jul 25, 2024

Thanks @alexrashed for the additional insights here! Agreed that LS runtime dependencies should not be included or pinned by extensions, makes total sense. 👍 (That's why we have rolo only as a test dependency in this extension - although some of the dependencies may actually be cleaned up in setup.cfg, we have a long-standing TODO in there which I was hoping to tackle some time soon). For now, we'll just get it merged and released as-is - @RobertLucian we can then work with the customer to upgrade to the latest release version. 👍

@whummer whummer merged commit 1e7ec82 into main Jul 25, 2024
1 check passed
@whummer whummer deleted the proxy-from-docker branch July 25, 2024 08:13
@RobertLucian
Copy link
Contributor

@alexrashed @whummer got it. The only problem I've noticed is that installing the latest rolo version that got released 2 days ago (0.7.0) breaks the AWS replicator extension with the Error: cannot import name 'Dispatcher' from 'rolo.router' (/usr/local/lib/python3.10/site-packages/rolo/router.py) error. Downgrading that to 0.6.1 fixes it.

What's the best way to ensure that the AWS Replicator extension works as expected and is straightforward to install and run?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants