-
Notifications
You must be signed in to change notification settings - Fork 480
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
Add git provenance labels #1297
Conversation
3226eef
to
10a40d0
Compare
There should probably be another annotation for the context directory path, right? (I am not a maintainer! Just an observation ❤️) |
Could you elaborate "annotation for the context directory path"? |
Sorry @tianon, somehow I missed your comment. Apologies for the late response.
We are resolving the Meaning if you build with Is that enough for what you had in mind? |
I mean something like Alternatively, something like |
@tianon, we are currently not storing the context path. So far this wasn't really needed for any of our use cases; we aren't trying to store all args etc to recreate the build command. It would be interesting to understand if you had a use case in mind for this?
In this case we would store
We would store |
The best I can come up with are use cases similar to wanting the (I don't feel strongly about this, that's just the only thing I noticed while looking at the annotations 👍) |
b88b3e8
to
be9f116
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.
Should buildx bake
commands also pick up these labels? In that case the code should be moved to the build pkg from commands(or a shared package that can be called from commands/build
and bake
pkg). And an additional test added to bake/bake_test
that checks that target picks up these labels.
1369f46
to
3abbaf3
Compare
bc379b2
to
a2e8d9e
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.
I think you missed #1297 (review) comment
a2e8d9e
to
f947ff6
Compare
@tonistiigi does this need to happen? In this PR or can this come after? If so, could you give some pointers? |
@cdupuis yup it should happen ideally - we want to avoid bake and buildx producing different results. My recommendation for how to split it up:
The code should pretty much stay the same, though the additional test in |
f947ff6
to
230a14c
Compare
Thanks @jedevc. I've made those changes. Let me know what you think. |
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.
LGTM.
@tonistiigi I'm not sure how we'd do a test here, since the existing tests test for ReadTargets
- we don't actually have an easy way to check the opts sent to the builder, either in BuildWithResultHandler
or in bake
and commands/build
- we don't have any tests for that at all anywhere in buildx. Loading them in ReadTargets
definitely doesn't feel right, since then the LABEL
s would appear in the --print
results. I'd be happy to merge without the test for now (we could revisit later).
c484bda
to
b8657ff
Compare
Why isn't it correct to show these in |
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 think the bake
part is wrong and should be handed in ReadTargets
so users see what config bake
produces. If these labels are already set by the config itself, then the environment variables do not override them. I'm fine with merging the current implementation and creating follow-up PR for the bake
refactor and test.
@tonistiigi, I'm happy to work on this in a subsequent PR. |
b8657ff
to
4039579
Compare
as per docker#1290 Signed-off-by: Christian Dupuis <cd@atomist.com>
4039579
to
e3c91c9
Compare
As discussed in #1290, this PR adds opt-in labels for Git provenance.
The following labels will be added when
BUILDX_GIT_LABELS=full
is set:With only
BUILDX_GIT_LABELS =1
the repository label will not be included:If the cloned source is not clean, the
revision
label value will get a-dirty
suffix:fixes #1290