-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Generate and embed build sources #2311
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thaJeztah @AkihiroSuda Please verify you're ok with the keys in the config structure.
I see some TODOs still left. In addition, needs integration tests.
Wonder if we should merge containerimage.buildinfo/* to a single containerimage.buildinfo key.
Different platforms can have different dependencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also need to add an exporter option for opt-out.
4e3d1f4
to
e9767fb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some random thoughts.
Also wondering; should this be a field in the image, or should they be stored as annotation / label? (label org.mobyproject.buildkit.buildinfo.v0=<data>
, or a com.docker.
reserved prefix. When doing so, we need to take into account that containerd limits label data to 4k (if I remember correctly)
6c85966
to
61158d5
Compare
This is not human-readable so it should produce extra noise for the user. I guess the inline cache variant also can easily hit size limits. |
61158d5
to
7fdd8cc
Compare
Agreed, it's not human readable; that said, labels were to add metadata, which not necessarily has to be human-readable. Wouldn't the same apply to the current field (showing up in |
Basically, my concern was that Will that cause issues? |
Don't think it should as this is BuildKit specific and unknown fields in the oci spec are skipped afaik. |
c9bff8b
to
1ef01a3
Compare
I do think labels/annotations are human-readable and searchable by definition. This is just noise as it is an encoded field. Possibly very large as well. Cache/opts will be larger than this buidinfo but they should all use the same pattern.
It should not show up in docker inspect atm (same as cache). Inspect does not show raw config.
Lots of Dockerfile fields are not. This is buildkit specific so we shouldn't use a field that could collide with something else. |
1633ae6
to
6f3c815
Compare
1000f5e
to
1bb9a73
Compare
f7b3770
to
bdb7ddb
Compare
125967e
to
cdd00d2
Compare
cdd00d2
to
06d50f1
Compare
@@ -173,12 +175,20 @@ func (s *Solver) Solve(ctx context.Context, id string, sessionID string, req fro | |||
} | |||
inp.Ref = workerRef.ImmutableRef | |||
|
|||
dt, err := inlineCache(ctx, exp.CacheExporter, r, session.NewGroup(sessionID)) | |||
dtbi, err := buildinfo.Merge(ctx, res.BuildInfo(), inp.Metadata[exptypes.ExporterImageConfigKey]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why can't this be just inside the imagewriter exporter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need the merged result for the exporter response too (metadata).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but metadata is also returned by the exporter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I done that in the solver to avoid changing the ExporterInstance.Export signature because ResultProxy is not passed to it. Do you see any downside about it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a strict requirement but thought it could be cleaner. You can test in a follow-up and see if it improves.
case *source.ImageIdentifier: | ||
for _, bi := range icbi { | ||
// Use original user input from image config | ||
if bi.Type == exptypes.BuildInfoTypeDockerImage && bi.Alias == sid.Reference.String() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this reference does not need TagNameOnly
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no we want to match the alias which is the actual ref.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is "actual ref" in here. TagNameOnly
would mean we normalize both sides.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pseudo-ref from alias sorry which is retrieved from ResolveImageConfig
in the frontend and will match the one from LLB.
07272d1
to
d269a38
Compare
4d50ac6
to
be2fc19
Compare
be2fc19
to
5b990eb
Compare
5b990eb
to
d990c9c
Compare
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
d990c9c
to
5fcc944
Compare
Fixes #2269
crazymax/buildkit:buildsources
crazymax/dockerfile:master
Create and boot builder:
Build image:
Image config generated with the above command:
moby.buildkit.buildinfo.v0
is a single base64 encoded string and will look like this with the above response:ExporterResponse
also contains a new keycontainerimage.buildinfo
if we want to use it in theclient.SolveResponse
:containerimage.buildinfo
is also a single base64 encoded string likemoby.buildkit.buildinfo.v0
.Multi-platform is also handled:
Each image config will have the same structure but
ExporterResponse
will have a key for each platform:Wonder if we should merge
containerimage.buildinfo/*
to a singlecontainerimage.buildinfo
key.