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

Add git provenance labels #1297

Merged
merged 1 commit into from
Sep 6, 2022
Merged

Add git provenance labels #1297

merged 1 commit into from
Sep 6, 2022

Conversation

cdupuis
Copy link
Contributor

@cdupuis cdupuis commented Aug 29, 2022

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:

      "Labels": {
        "com.docker.image.source.entrypoint": "Dockerfile",
        "org.opencontainers.image.revision": "5734329c6af43c2ae295010778cd308866b95d9b",
        "org.opencontainers.image.source": "git@github.com:atomisthq/kipz-docker-test.git"
      }

With only BUILDX_GIT_LABELS =1 the repository label will not be included:

      "Labels": {
        "com.docker.image.source.entrypoint": "Dockerfile",
        "org.opencontainers.image.revision": "5734329c6af43c2ae295010778cd308866b95d9b"
      }

If the cloned source is not clean, the revision label value will get a -dirty suffix:

      "Labels": {
        "com.docker.image.source.entrypoint": "Dockerfile",
        "org.opencontainers.image.revision": "5734329c6af43c2ae295010778cd308866b95d9b-dirty"
      }

fixes #1290

@cdupuis cdupuis force-pushed the git-revision branch 4 times, most recently from 3226eef to 10a40d0 Compare August 29, 2022 14:42
@tianon
Copy link
Contributor

tianon commented Aug 29, 2022

There should probably be another annotation for the context directory path, right?

(I am not a maintainer! Just an observation ❤️)

@AkihiroSuda
Copy link
Collaborator

There should probably be another annotation for the context directory path, right?

Could you elaborate "annotation for the context directory path"?

@cdupuis
Copy link
Contributor Author

cdupuis commented Aug 31, 2022

Sorry @tianon, somehow I missed your comment. Apologies for the late response.

There should probably be another annotation for the context directory path, right?

We are resolving the Dockerfile path against the root of the Git repository.

Meaning if you build with docker buildx build somepath -f Dockerfile -t some:tag then we would record somepath/Dockerfile if the current working directory is the root of Git repository.

Is that enough for what you had in mind?

@tianon
Copy link
Contributor

tianon commented Aug 31, 2022

I mean something like docker build -f dir1/Dockerfile dir2 (where both dir1/Dockerfile and dir2 are relevant bits of information, and both are required to figure out how the build happened).

Alternatively, something like docker build -f ../foo/Dockerfile . or docker build -f docker/Dockerfile . (which is probably a more common case) -- basically any instance where the Dockerfile does not sit at the root of the build context.

commands/build.go Outdated Show resolved Hide resolved
commands/build.go Outdated Show resolved Hide resolved
commands/build.go Outdated Show resolved Hide resolved
commands/build.go Outdated Show resolved Hide resolved
@cdupuis
Copy link
Contributor Author

cdupuis commented Aug 31, 2022

@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?

I mean something like docker build -f dir1/Dockerfile dir2 (where both dir1/Dockerfile and dir2 are relevant bits of information, and both are required to figure out how the build happened).

In this case we would store dir1/Dockerfile in the com.docker.image.dockerfile.path label, given current working directory is the root of the Git repository.

Alternatively, something like docker build -f ../foo/Dockerfile . or docker build -f docker/Dockerfile . (which is probably a more common case) -- basically any instance where the Dockerfile does not sit at the root of the build context.

We would store foo/Dockerfile or docker/Dockerfile in the com.docker.image.dockerfile.path label, given . is also the root of the Git repository.

@tianon
Copy link
Contributor

tianon commented Aug 31, 2022

The best I can come up with are use cases similar to wanting the Dockerfile path - imagine COPY foo.sh /... being able to link directly back to (the correct) foo.sh 😅

(I don't feel strongly about this, that's just the only thing I noticed while looking at the annotations 👍)

@cdupuis cdupuis force-pushed the git-revision branch 2 times, most recently from b88b3e8 to be9f116 Compare August 31, 2022 17:49
Copy link
Member

@tonistiigi tonistiigi left a 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.

commands/build.go Outdated Show resolved Hide resolved
commands/build.go Outdated Show resolved Hide resolved
commands/build.go Outdated Show resolved Hide resolved
commands/build.go Outdated Show resolved Hide resolved
commands/build_test.go Outdated Show resolved Hide resolved
commands/build.go Outdated Show resolved Hide resolved
commands/build.go Outdated Show resolved Hide resolved
commands/build.go Outdated Show resolved Hide resolved
commands/build.go Outdated Show resolved Hide resolved
commands/build.go Outdated Show resolved Hide resolved
@cdupuis cdupuis force-pushed the git-revision branch 2 times, most recently from 1369f46 to 3abbaf3 Compare September 1, 2022 09:59
commands/build.go Outdated Show resolved Hide resolved
commands/build.go Outdated Show resolved Hide resolved
commands/build.go Outdated Show resolved Hide resolved
@cdupuis cdupuis force-pushed the git-revision branch 6 times, most recently from bc379b2 to a2e8d9e Compare September 1, 2022 14:43
Copy link
Member

@tonistiigi tonistiigi left a 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

commands/build.go Outdated Show resolved Hide resolved
commands/build.go Outdated Show resolved Hide resolved
commands/build_test.go Outdated Show resolved Hide resolved
@cdupuis
Copy link
Contributor Author

cdupuis commented Sep 2, 2022

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.

@tonistiigi does this need to happen? In this PR or can this come after? If so, could you give some pointers?

@jedevc
Copy link
Collaborator

jedevc commented Sep 2, 2022

@cdupuis yup it should happen ideally - we want to avoid bake and buildx producing different results.

My recommendation for how to split it up:

  • Add the git helpers to build/git.go, and the tests to build/git_test.go
  • Call addGitProvenance from BuildWithResultHandler and modify all the opts structures in the opts map[string]BuildOpts with the correct labels, using the BuildOpts.Inputs

The code should pretty much stay the same, though the additional test in bake_test.go with an e2e test would be super useful (happy to take a look at that if you let me know).

@cdupuis
Copy link
Contributor Author

cdupuis commented Sep 2, 2022

Thanks @jedevc. I've made those changes. Let me know what you think.

Copy link
Collaborator

@jedevc jedevc left a 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 LABELs would appear in the --print results. I'd be happy to merge without the test for now (we could revisit later).

build/build.go Outdated Show resolved Hide resolved
build/git.go Outdated Show resolved Hide resolved
@cdupuis cdupuis force-pushed the git-revision branch 2 times, most recently from c484bda to b8657ff Compare September 2, 2022 13:28
@tonistiigi
Copy link
Member

Loading them in ReadTargets definitely doesn't feel right, since then the LABELs would appear in the --print results.

Why isn't it correct to show these in --print results? That should show the configuration of the build that is sent to the daemon, doesn't matter if it is loaded from files, --set or from environment variables.

Copy link
Member

@tonistiigi tonistiigi left a 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.

@cdupuis
Copy link
Contributor Author

cdupuis commented Sep 6, 2022

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.

build/build.go Outdated Show resolved Hide resolved
as per docker#1290

Signed-off-by: Christian Dupuis <cd@atomist.com>
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.

Proposal: Attach Git information via image labels
8 participants