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 docker-compose files for lms #182

Merged
merged 1 commit into from
Nov 23, 2021

Conversation

andrewkrug
Copy link
Collaborator

This pull request fixes a problem introduced in 3199fe5 where patches were added in favor of reducing code duplication. The docker-compose files formerly leveraged a shared volume with each code set. Now that the patches are applied to the containers themselves this PR does the following:

Adds an entrypoint outside of the app working directory to /opt/storedog/
Removes volume share from the docker-compose itself
Removes explicit command override in favor of letting the container default to the CMD in the Dockerfile

@andrewkrug
Copy link
Collaborator Author

Tested and works locally for the security scenario. Others should be tested. But they have likely been broken for some time in docker-compose. A follow up action should also be to update any of the other files that override the CMD for the storedog containers.

Copy link
Collaborator

@stanleegoodspeed stanleegoodspeed left a comment

Choose a reason for hiding this comment

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

I was able to successfully spin up these compose files locally, so these changes LGTM.

  • deploy/docker-compose/docker-compose-fixed-instrumented-attack.yml
  • deploy/docker-compose/docker-compose-fixed-instrumented.yml
  • deploy/docker-compose/docker-compose-broken-instrumented.yml
  • deploy/docker-compose/docker-compose-broken-no-instrumentation.yml
  • deploy/docker-compose/docker-compose-broken-no-apm-instrumentation.yml

FWIW, I'm also able to spin these up using the current main branch. So I'm a little unclear on how to "break" the current version

@jeremy-lq jeremy-lq merged commit d8eab89 into DataDog:main Nov 23, 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
Development

Successfully merging this pull request may close these issues.

3 participants