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: use renovate/yarn as base image #4710

Merged
merged 5 commits into from
Oct 25, 2019
Merged

feat: use renovate/yarn as base image #4710

merged 5 commits into from
Oct 25, 2019

Conversation

viceice
Copy link
Member

@viceice viceice commented Oct 23, 2019

This pr uses renovate/yarn as base image.

So i removed node, npm and yarn.
I also adapted the multistage build to match the slim image

@viceice viceice requested a review from rarkins October 23, 2019 07:27
@viceice
Copy link
Member Author

viceice commented Oct 23, 2019

relates to #4708

@viceice viceice changed the title feat: update yarn docker image to node lts v12 feat: update yarn docker image to node lts v12 (wip= Oct 23, 2019
@viceice viceice changed the title feat: update yarn docker image to node lts v12 (wip= feat: update yarn docker image to node lts v12 (wip) Oct 23, 2019
@viceice viceice changed the title feat: update yarn docker image to node lts v12 (wip) feat: use renovate/yarn as base image Oct 23, 2019
Copy link
Collaborator

@rarkins rarkins left a comment

Choose a reason for hiding this comment

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

I don't think our CI actually tests building so we need to test this manually too

Dockerfile Outdated
# required for install
USER root

RUN chown -R ubuntu:ubuntu /usr/src/app/
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure this achieves anything here, unless we built renovate/yarn badly?

Copy link
Member Author

Choose a reason for hiding this comment

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

maybe we no longer need to write to this path?

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK let's remove this section and see if everything is still fine

Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Copy link
Member Author

@viceice viceice left a comment

Choose a reason for hiding this comment

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

Have tested it locally against gitlab with docker and yarn manger

Dockerfile Outdated
# required for install
USER root

RUN chown -R ubuntu:ubuntu /usr/src/app/
Copy link
Member Author

Choose a reason for hiding this comment

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

maybe we no longer need to write to this path?

Dockerfile Outdated Show resolved Hide resolved
@rarkins rarkins changed the base branch from master to v20 October 24, 2019 07:28
@rarkins
Copy link
Collaborator

rarkins commented Oct 24, 2019

Eventually - maybe v21 - I want to stop bundling npm, yarn and pnpm with Renovate (i.e. remove them as npm dependencies). Until then, we are installing npm and yarn as part of "yarn install" anyway so I'm not sure if we're saving much versus using renovate/node as our base image for publishing instead?

@viceice
Copy link
Member Author

viceice commented Oct 24, 2019

We do not save space for now, but we save some build time and both images have the same base.

And after this it is mode easy to migrate to node v12, because we only need to update base image

When installing nodejs we always have npm installed, bcause it is packaged in nodejs.

@rarkins
Copy link
Collaborator

rarkins commented Oct 24, 2019

How would it be worse though if the final image was based off renovate/node instead of renovate/yarn? renovate/node should be a subset of renovate/yarn, and we don't run any yarn x commands anyway. And we'd save a little bit of space by not having yarn installed "twice" in the final image (once in renovate/yarn, once by yarn install renovate).

@viceice
Copy link
Member Author

viceice commented Oct 24, 2019

🤔 We can use the renovate/node for final image, but for build we need yarn.

@viceice
Copy link
Member Author

viceice commented Oct 24, 2019

If we use different base images we have to make sure, that both have the same node version.

also yarn only add 2mb (compressed) to the image

@rarkins
Copy link
Collaborator

rarkins commented Oct 24, 2019

Ok fine, keep as is

@viceice
Copy link
Member Author

viceice commented Oct 24, 2019

So merge to v20?

@viceice viceice requested a review from rarkins October 24, 2019 16:42

USER root

# Python 2 and make are required to build node-re2
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't we be on python 3 now?

@rarkins rarkins merged commit b72a45f into v20 Oct 25, 2019
@rarkins rarkins deleted the feat/docker-update-node branch October 25, 2019 10:28
viceice added a commit that referenced this pull request Feb 19, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants