Skip to content

Conversation

@pinzon
Copy link
Member

@pinzon pinzon commented Jan 12, 2024

This pull request addresses the issue outlined in #153, where LocalStack was inadvertently initiating a container despite the plugin being inactive.

Additionally, it introduces a new capability (requested in #75): instead of initiating a container through the LocalStack CLI or Docker bin, users can now specify a Docker Compose file to be utilized during the autostart phase. This enhancement provides more flexibility and control over the containerization process.

@pinzon pinzon changed the title wip: autostart and docker compose fix autostart and add support for docker compose Jan 22, 2024
@pinzon pinzon marked this pull request as ready for review January 22, 2024 21:24
@pinzon pinzon requested review from joe4dev, steffyP and whummer and removed request for steffyP and whummer January 22, 2024 21:24
Copy link
Member

@steffyP steffyP left a comment

Choose a reason for hiding this comment

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

Nice set of changes @pinzon! Having this docker-compose feature will certainly be nice for a lot of people, and it makes the serverless plugin more user-friendly 🥳

We already went to some things together and I continued testing afterwards, here are some things I found (some we already discussed):

  • for the autouse (old docker functionality): the -d flag is not working on my docker version. the docker create does not have an option -d, maybe this is a leftover? If you have the same issue, I would like to fix this with this PR as well

  • there is a docker-compose.yml on the root level of this project - I think we should adapt it as well with this PR, so that people actually have an up-to-date template of the compose file

  • the start of the docker-compose file isn't working for me, I had to add up -d in order to make it work (see also inline comment)

  • we may have another issue with the the getContainer function, as the image name may be localstack/localstack-pro. I think we should add a check here and try both image names
    Line #416:

const getContainer = () => { return exec('docker ps').then( (stdout) => { const exists = stdout.split('\n').filter((line) => line.indexOf('localstack/localstack') >= 0 || line.indexOf('localstack_localstack') >= 0); if (exists.length) { return exists[0].replace('\t', ' ').split(' ')[0]; } } ) }; 
@pinzon pinzon requested a review from steffyP January 24, 2024 18:33
Copy link
Member

@steffyP steffyP left a comment

Choose a reason for hiding this comment

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

Great work @pinzon! 👏
Thanks for jumping on my comments so fast.

There are just some minor issues left to resolve, then I think we are ready to merge 🎉

pinzon and others added 3 commits January 26, 2024 12:43
Co-authored-by: steffyP <steffyP@users.noreply.github.com>
Co-authored-by: steffyP <steffyP@users.noreply.github.com>
@pinzon pinzon merged commit 648396b into master Jan 26, 2024
@whummer whummer deleted the feat/autostart-dcompose branch January 29, 2024 08:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants