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

Remove no-op multistage builds from Dockerfiles #379

Closed
wants to merge 1 commit into from

Conversation

xurizaemon
Copy link
Contributor

NB: I may be misunderstanding the intended outcome so please verify my proposed change carefully 😄

AFAICT, a FROM statement without a following as <name>, as seen here, will have no effect on the built image. (I expect the image gets pulled, but that seems like a side-effect here, not an output?)

So - I think these extra FROM are redundant and can be discarded. My understanding is that if they were followed by a name (FROM nodejs as node) and had separate build instructions, that would be a multi-stage build, but that as-is these are a multi-stage build with a single output, and that the state is cleared when the second FROM command is reached.

Copy link
Contributor

@joecorall joecorall left a comment

Choose a reason for hiding this comment

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

These aren't as they appear, as they're getting aliased in docker-bake.hcl e.g. https://github.com/Islandora-Devops/isle-buildkit/blob/main/docker-bake.hcl#L315-L322

They get built in separate repos e.g. https://github.com/Islandora-Devops/isle-nodejs

Semi-related: I have plans to get these auto-updated using renovate and have the FROM in dockerfiles be better documented avoiding confusion such as this PR, but haven't had time to put up that PR yet. I started a while ago over at https://github.com/joecorall/isle-buildkit but it's nowhere close to ready. I should have time to push that forward this week

@joecorall
Copy link
Contributor

Also, the images do get used in the build e.g

--mount=type=bind,from=nodejs,source=/packages,target=/packages \

@nigelgbanks
Copy link
Contributor

Semi-related: I have plans to get these auto-updated using renovate and have the FROM in dockerfiles be better documented avoiding confusion such as this PR, but haven't had time to put up that PR yet. I started a while ago over at https://github.com/joecorall/isle-buildkit but it's nowhere close to ready. I should have time to push that forward this week

@joecorall Before you sink time into that, those repositories are likely to be merged into the isle-buildkit. I've solved a number of the caching issues, which was the reason they were split into their own repositories in the first place. I'm still working away it maybe another week or two before I'm ready with a pull request, though.

@xurizaemon
Copy link
Contributor Author

Thanks so much for documenting and closing this, and apologies for any bother! Much appreciate the insight.

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.

3 participants