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 autostart and add support for docker compose #242

Merged
merged 12 commits into from
Jan 26, 2024

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 steffyP, whummer and joe4dev 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];
          }
        }
      )
    };

src/index.js Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
example/service/test/serverless.yml Outdated Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
@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 🎉

package-lock.json Outdated Show resolved Hide resolved
example/service/serverless.yml Outdated Show resolved Hide resolved
src/index.js Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
@pinzon pinzon merged commit 648396b into master Jan 26, 2024
2 checks passed
@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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants