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

Multistage ONBUILD COPY Support #1190

Merged
merged 5 commits into from
Apr 15, 2020
Merged

Conversation

dani29
Copy link
Contributor

@dani29 dani29 commented Apr 12, 2020

Fixes #533.

Description

ONBUILD did not work correctly, because the resolveOnBuild() method occurred after the resolveStages() method was called, so commands like COPY --from=builder were not properly replaced with --from=0. Moreover, because the CalculateDependency() method were unaware of these dependencies, files copied onbuild weren't even registered to be kept beyond the stage they were used it.

I had to break down dockerfile.Stages(opts), which was a huge method, into smaller chunks doing the following:

  1. parsing the dockerfile into []instructions.Stage (no changes here)
  2. resolving instructions within stages with references to previous stages (keeping a mapping of stageName:stageIndex)
  3. building KanikoStages from the []instructions.Stage (no changes here)

afterwards, adding the following:

  • passing the map from step 2 to CalculateDependency() so it would register dependencies for ONBUILD files
  • passing the map from step 2 to NewStageBuilder -> resolveOnBuild(), so the COPY commands "From" will be replaced with the stage index.

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

  • Includes unit tests
  • Adds integration tests if needed.

See the contribution guide for more details.

Reviewer Notes

  • The code flow looks good.
  • Unit tests and or integration tests added.

Release Notes

Describe any changes here so maintainer can include it in the release notes, or delete this block.

Examples of user facing changes:
- Support ONBUILD Copy in multistage builds (#533)

@googlebot googlebot added the cla: yes CLA signed by all commit authors label Apr 12, 2020
@dani29 dani29 changed the title WIP: Multistage ONBUILD COPY Support Multistage ONBUILD COPY Support Apr 12, 2020
@dani29 dani29 requested a review from tejal29 April 12, 2020 10:11
@dani29 dani29 force-pushed the onbuildcp branch 2 times, most recently from 8e301b2 to 4134965 Compare April 12, 2020 11:07
@tejal29
Copy link
Member

tejal29 commented Apr 12, 2020

@dani29 the code flows looks good.
Thanks for the refactoring!

@@ -52,6 +52,11 @@ import (
// This is the size of an empty tar in Go
const emptyTarSize = 1024

// for testing
var (
InitializeConfig = initializeConfig
Copy link
Member

Choose a reason for hiding this comment

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

Please make this a local variable so only the tests in build package can override this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

Copy link
Member

@tejal29 tejal29 left a comment

Choose a reason for hiding this comment

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

LGTM, except 1 nit.
Also can you please mention, did you test this PR on the Dockerfile in #533

@dani29
Copy link
Contributor Author

dani29 commented Apr 12, 2020

Yes, I've verified that the Dockerfile provided by @mbfisher in #533 can be built.
The other docker file did not have the same issue with ONBUILD.

@dani29
Copy link
Contributor Author

dani29 commented Apr 15, 2020

@JordanGoasdoue, I've had to manually fix conflicts since git wasn't able to digest the changes of both this PR and #1160.
Any chance you could take a quick look and make sure I mess up anything with your flow?

@JordanGoasdoue
Copy link
Contributor

Done @dani29 , it's all good

@dani29 dani29 merged commit 1534f90 into GoogleContainerTools:master Apr 15, 2020
@dani29 dani29 deleted the onbuildcp branch April 15, 2020 18:30
@kirkvogen
Copy link

Thank you for making this fix. I’ve been playing around with Kaniko and ran into this issue today. And then I do a search and find that you fixed it 15 minutes ago. Serendipity!

@kirkvogen
Copy link

I tried the latest releases of the image. It fixed the issue that I was seeing with multi-stage ONBUILD. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes CLA signed by all commit authors
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ONBUILD COPY fails using multi-stage build
5 participants