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

feat: impr dockerfile build speed; use entrypoint #195

Draft
wants to merge 1 commit into
base: stable
Choose a base branch
from

Conversation

linuxbandit
Copy link
Member

Here it is finally, the 'good dockerfile' I had mentioned somewhere (can you link the issue? so we have some cross-reference).

There are 2 changes here,
first is that the files are copied last, so that the cache layer of npm install is not invalidated. This means faster builds.
second is the use of ENTRYPOINT along CMD. it's just a good trick to make a docker container execute different stuff based on the argument passed (you see that in dev we add | bunyan --color ).

In general, I am not sure about the use of nodemon, discuss

@WikiRik
Copy link
Member

WikiRik commented Dec 8, 2020

AEGEE/statutory#447

@@ -1,29 +1,35 @@
# FIXME use alpine
Copy link
Member

Choose a reason for hiding this comment

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

Why would alpine be better?

Copy link
Member Author

Choose a reason for hiding this comment

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

not better but more "like a pro" (if you use lightweight containers is better, less footprint. Of course this is most important in systems we do not have, like kubernetes, but we want to be following good practices so it's good to mark this)

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good, then I would suggest making line 2 into FROM node:14.15.1-alpine

Copy link
Member Author

Choose a reason for hiding this comment

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

yes I didn't do it myself because we don't need that level of lightweight-ness yet, and I therefore didn't want to risk breaking image building et al ('larger' image means that we have more/different tools, e.g. we do not use apt-get but apk, and i didn't want to spend time on that)

&& mkdir -p /usr/app/scripts
&& mkdir -p /usr/app/scripts \
&& apt-get update \
&& apt-get install netcat -y
Copy link
Member

Choose a reason for hiding this comment

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

I believe @serge1peshcoff mentioned that we do not need this anymore, and we could also remove the wait script

USER node

ENV NPM_CONFIG_PREFIX=/home/node/.npm-global
ENV PATH="/home/node/.npm-global/bin:${PATH}"

RUN npm install -g --loglevel warn nodemon bunyan && npm cache clean --force
RUN npm install --loglevel warn
# FIXME no need for nodemon
Copy link
Member

Choose a reason for hiding this comment

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

I would keep nodemon for dev, but we can remove it in prod I guess

Copy link
Member Author

Choose a reason for hiding this comment

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

yes as mentioned above, there's the need for a discussion. Now I am trying to close as much as possible :D

docker/docker-compose.dev.yml Show resolved Hide resolved
@WikiRik
Copy link
Member

WikiRik commented Dec 8, 2020

Might just be me, but I wanted to test it using make rebuild_core and it's just constantly restarting. Any clue why?

@linuxbandit
Copy link
Member Author

Might just be me, but I wanted to test it using make rebuild_core and it's just constantly restarting. Any clue why?

what's the error code/message?

@WikiRik
Copy link
Member

WikiRik commented Dec 9, 2020

It's just looping through bootstrap.sh in two cycles it seems. One time everything is fine and the other time it's complaining that the database already exists

@WikiRik
Copy link
Member

WikiRik commented Dec 9, 2020

Update, I 'fixed' it by removing the entrypoint. For some reason Docker just keeps executing the bootstrap.sh

@WikiRik
Copy link
Member

WikiRik commented Feb 8, 2021

@linuxbandit I've done these changes for statutory and the result is weird;

this works

CMD ["sh", "/usr/app/scripts/bootstrap.sh && nodemon -e 'js,json' lib/run.js"]

this does not

ENTRYPOINT [ "/usr/app/scripts/bootstrap.sh" ]
CMD ["nodemon -e 'js,json' lib/run.js"]

neither does this

ENTRYPOINT [ "/usr/app/scripts/bootstrap.sh" ]
CMD ["sh", "nodemon -e 'js,json' lib/run.js"]

@WikiRik WikiRik marked this pull request as draft March 15, 2021 12:37
@linuxbandit
Copy link
Member Author

very last attempt, you wrote CMD ["sh", "nodemon -e 'js,json' lib/run.js"], try CMD ["sh", "nodemon", "-e", "'js,json'", "lib/run.js"]

@WikiRik
Copy link
Member

WikiRik commented Mar 20, 2021

very last attempt, you wrote CMD ["sh", "nodemon -e 'js,json' lib/run.js"], try CMD ["sh", "nodemon", "-e", "'js,json'", "lib/run.js"]

I can try, but I'm hesitant. The thing that I said works, does work for me locally but not on prod.

@WikiRik
Copy link
Member

WikiRik commented Apr 5, 2021

very last attempt, you wrote CMD ["sh", "nodemon -e 'js,json' lib/run.js"], try CMD ["sh", "nodemon", "-e", "'js,json'", "lib/run.js"]

Failed on my server, permission denied to execute stuff for /usr/app/scripts/bootstrap.sh

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