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: bind-mount in dev-specific services #713

Merged
merged 2 commits into from
Jul 29, 2022
Merged

Conversation

regisb
Copy link
Contributor

@regisb regisb commented Jul 25, 2022

The -m/--mount option makes it possible to bind-mount volumes at runtime. The
volumes are declared in a local/docker-compose.tmp.yml file. The problem with
this approach is when we want to bind-mount a volume to a service which is
specific to the dev context. For instance: the "learning" service when the MFE
plugin is enabled.

In such a case, starting the service triggers a call to docker-compose stop
in the local context. This call fails because the "learning" service does not
exist in the local context. Note that this issue only seems to occur with
docker-compose v1.

To resolve this issue, we create two additional filters for
the dev context, which emulate the behaviour of the local context. With this approach, we convert the -m/--mount arguments right after they are parsed. Because they are parsed just once, we can get rid of the de-duplication logic initially introduced with the COMPOSE_CLI_MOUNTS context.

Close #711. Close also overhangio/tutor-mfe#57.

@regisb
Copy link
Contributor Author

regisb commented Jul 25, 2022

@kdmccormick I have attempted to refactor, and in the process simplify the bind-mounting code. As a consequence I have removed the filter callback de-duplication code we just merged :-/ I understand we could have spared the effort if I had worked on this earlier, but I only became aware of #711 in the past few days...

@dagg this PR should resolve your issue with bind-mounting the learning MFE.

@kdmccormick
Copy link
Collaborator

I understand we could have spared the effort if I had worked on this earlier, but I only became aware of #711 in the past few days...

@regisb All good, these things happen :) If you needed my review, I am about to be done for the day but could take a look tomorrow.

Copy link
Collaborator

@kdmccormick kdmccormick left a comment

Choose a reason for hiding this comment

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

Sorry for the delayed review.

@regisb regisb force-pushed the regisb/fix-learning-mount branch 2 times, most recently from dfe18f5 to 9bddf14 Compare July 29, 2022 17:28
Copy link
Collaborator

@kdmccormick kdmccormick left a comment

Choose a reason for hiding this comment

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

In addition to the bugfix, I think this was a much-needed refactor. Looks great.

regisb added 2 commits July 29, 2022 19:48
The -m/--mount option makes it possible to bind-mount volumes at runtime. The
volumes are declared in a local/docker-compose.tmp.yml file. The problem with
this approach is when we want to bind-mount a volume to a service which is
specific to the dev context. For instance: the "learning" service when the MFE
plugin is enabled.

In such a case, starting the service triggers a call to `docker-compose stop`
in the local context. This call fails because the "learning" service does not
exist in the local context. Note that this issue only seems to occur with
docker-compose v1.

To resolve this issue, we create two additional filters for
the dev context, which emulate the behaviour of the local context. With this approach, we convert the -m/--mount arguments right after they are parsed. Because they are parsed just once, we can get rid of the de-duplication logic initially introduced with the COMPOSE_CLI_MOUNTS context.

Close #711. Close also overhangio/tutor-mfe#57.
@regisb regisb force-pushed the regisb/fix-learning-mount branch from 9bddf14 to 6b29f22 Compare July 29, 2022 17:49
@regisb
Copy link
Contributor Author

regisb commented Jul 29, 2022

Hey Kyle, I'm going to be away from keyboard for a few weeks, so I'm going to merge this now. There are a few important bugfixes in the changelog, so I'll create a new release (v14.0.4). If there are important issues in this new release, please let me know by email. If there is a PR with a fix and a new version I should be able to review and merge it. Thanks!

@regisb regisb merged commit 52cf0cc into master Jul 29, 2022
@regisb regisb deleted the regisb/fix-learning-mount branch July 29, 2022 17:53
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.

Bind mounting MFEs fails on dev
2 participants