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

🐳 fixing docker build to use committed code #855

Merged
merged 4 commits into from
Jul 1, 2019
Merged

Conversation

ISNIT0
Copy link
Contributor

@ISNIT0 ISNIT0 commented Jun 28, 2019

Closes #722

@codecov
Copy link

codecov bot commented Jun 28, 2019

Codecov Report

Merging #855 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #855   +/-   ##
=======================================
  Coverage   57.43%   57.43%           
=======================================
  Files          19       19           
  Lines        1588     1588           
  Branches      322      322           
=======================================
  Hits          912      912           
  Misses        517      517           
  Partials      159      159

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a9202a5...1188a73. Read the comment docs.

@kelson42
Copy link
Collaborator

@ISNIT0 Why no CI report here?

@kelson42
Copy link
Collaborator

This seems to fail for me:

$ docker build docker/ -t test
Sending build context to Docker daemon  5.632kB
Step 1/13 : FROM node:10
 ---> 90fa0611d45b
Step 2/13 : RUN apt update && apt install -y --no-install-recommends make g++ curl git
 ---> Using cache
 ---> eaa1975ed541
Step 3/13 : RUN apt install -y --no-install-recommends imagemagick
 ---> Using cache
 ---> b630fea25d78
Step 4/13 : RUN npm --global config set user root
 ---> Using cache
 ---> 381d74b826e2
Step 5/13 : WORKDIR /mwoffliner
 ---> Using cache
 ---> 592e3fce4edf
Step 6/13 : COPY package*.json ./
COPY failed: no source files were specified

@ISNIT0
Copy link
Contributor Author

ISNIT0 commented Jun 29, 2019

@kelson42 Can we have a look at the dockerhub configuration please? Looks like the build context is wrong.

@ISNIT0
Copy link
Contributor Author

ISNIT0 commented Jun 29, 2019

The CI is running, click "Show all checks"

@kelson42
Copy link
Collaborator

@ISNIT0 What is the local command to build the image?

@ISNIT0
Copy link
Contributor Author

ISNIT0 commented Jun 29, 2019

From that directory: docker build . -f docker/Dockerfile

@kelson42
Copy link
Collaborator

This does not work for me and the Dockerfile is broken, but I will have a look.

@kelson42
Copy link
Collaborator

@ISNIT0 I have fixed the Dockerfile. I believe the hub.docker.com is properly configured, but we will have to see that when we are so far.

@ISNIT0
Copy link
Contributor Author

ISNIT0 commented Jul 1, 2019

@kelson42
It seems you've replaced COPY . . with lots of individual copies... This is not idea. Please just use the .dockerignore file that I've already created.

@kelson42
Copy link
Collaborator

kelson42 commented Jul 1, 2019

@ISNIT0 I prefer to be explicit here. Relying only on the .dockerignore is implicit copying which leads to the following situation:

  • Difficult to immediately know what is copied or not to the image
  • Does not secure that you never publish here something which is not intended
  • No way to forget to maintain the COPY instructions (which might be perfectly the case with the .dockerignore)
  • Not more work to maintain the COPY instructions in the Dockerfile than to maintain the .dockerignore

That said:

  • I might COPY too much?
  • Not against keeping the .dockerignore like it is.

@ISNIT0
Copy link
Contributor Author

ISNIT0 commented Jul 1, 2019

Using .dockerignore is explicitly listed under Docker best practices.
https://docs.docker.com/develop/develop-images/dockerfile_best-practices/#exclude-with-dockerignore

If you want to keep the multiple COPYs, it's best to remove the .dockerignore as it's confusing.

Please merge when you're happy with it.

@ISNIT0 ISNIT0 closed this Jul 1, 2019
@ISNIT0 ISNIT0 reopened this Jul 1, 2019
@kelson42
Copy link
Collaborator

kelson42 commented Jul 1, 2019

I want to keep the .dockerignore like you made it. This is a security.

@kelson42 kelson42 merged commit b9e0fb2 into master Jul 1, 2019
@kelson42 kelson42 deleted the ISNIT0/docker-build branch July 1, 2019 17:52
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.

Build mwoffliner from source in Docker
2 participants