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: include submodule bare repos in docker build context #5118

Merged
merged 1 commit into from
Apr 12, 2019

Conversation

focusaurus
Copy link
Contributor

Impact: minor
Type: bugfix|chore

Issue

OK this one is a pretty long chain to understand, but here goes. If you bundle your custom reaction plugins by adding them as git submodules, and one of your plugins depends on a package directly from github, it seems to cause docker builds to fail during the npm install. For some reason, in this scenario npm wants to access the submodule's gitdir (no idea what for). Given the globs in the .dockerignore as is, that directory does not exist in the docker container during the build.

Solution

Add an exclusion line to .dockerignore so the necessary directories are part of the docker build context.

Breaking changes

None

Testing

cd $(mktemp -d /tmp/submod-XXX)
git init repro
cat <<EOF > repro/package.json
{
  "name": "repro",
  "version": "1.0.0",
  "dependencies": {
    "epimetheus": "github:reactioncommerce/node-epimetheus#baseurl-undefined"
  }
}
EOF
cd repro
git add .
git commit -m "1"
cd ..
git clone https://github.com/reactioncommerce/reaction.git
cd reaction
git submodule add -f "${PWD}/../repro" imports/plugins/custom/repro
  • copy your main reaction .env into this test reaction directory
  • stop any other reaction containers that may be running
docker-compose build
docker run --rm -it reaction_reaction bash

OK now you're in the container which is kind of like how it is in some CI environments.

cd imports/plugins/custom/repro
npm i

You'll get this error:

npm ERR! Error while executing:
npm ERR! /usr/bin/git ls-remote -h -t ssh://git@github.com/reactioncommerce/node-epimetheus.git
npm ERR! 
npm ERR! fatal: Not a git repository: ../../../../.git/modules/imports/plugins/custom/repro
npm ERR! 
npm ERR! exited with error code: 128

npm ERR! A complete log of this run can be found in:
npm ERR!     /home/node/.npm/_logs/2019-04-12T02_37_02_115Z-debug.log

@focusaurus focusaurus added the bug For issues that describe a defect or regression in the released software label Apr 12, 2019
@focusaurus focusaurus self-assigned this Apr 12, 2019
Copy link
Contributor

@aldeed aldeed left a comment

Choose a reason for hiding this comment

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

I'd add a comment line above it with text similar to your explanation in this PR description. I know any of us looking at this in a year will probably think that line wrong and not remember why it's there.

But other than that, the change seems specific and safe. I didn't test it.

Signed-off-by: Peter Lyons <pete@reactioncommerce.com>
@focusaurus focusaurus force-pushed the fix-plugin-submodules-docker-npm branch from b55e959 to 7ad8fd2 Compare April 12, 2019 19:25
@focusaurus
Copy link
Contributor Author

Good idea. Comment added.

@focusaurus focusaurus requested a review from aldeed April 12, 2019 19:26
Copy link
Contributor

@rosshadden rosshadden left a comment

Choose a reason for hiding this comment

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

This looks okay to me, not knowing too much about it other than what you said and my own light research. It took a while to run everything, but I was able to reproduce both the problem and your fix.

@focusaurus focusaurus merged commit 8b3ba95 into develop Apr 12, 2019
@focusaurus focusaurus deleted the fix-plugin-submodules-docker-npm branch April 12, 2019 21:39
@jeffcorpuz jeffcorpuz mentioned this pull request Jul 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug For issues that describe a defect or regression in the released software
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants