fix .dockerignore to avoid dirty working tree

[crazy-max]

Fixes #3347
Closes #3348

cc @thaJeztah @tonistiigi

[codecov-commenter]

Codecov Report

Merging #3349 (83c2537) into master (e57b5f7) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #3349   +/-   ##
=======================================
  Coverage   58.01%   58.01%           
=======================================
  Files         302      302           
  Lines       21762    21762           
=======================================
  Hits        12625    12625           
  Misses       8215     8215           
  Partials      922      922           

[thaJeztah]

Can we somehow make the builds work without having to copy the .git directory? It feels like this is hiding the actual issue (i.e., the build now needs the git repository, but effectively just to generate a version)

[tonistiigi]

Did you verify that the changes were only from ignored files? I have a vague memory that it was always the case because of generated code but maybe that has been already fixed. I think in releases we ignore it because of VERSION override.

Can we somehow make the builds work without having to copy the .git directory? It feels like this is hiding the actual issue (i.e., the build now needs the git repository, but effectively just to generate a version)

I don't see any issue with that. It is working correctly by letting us know that there was an issue with validating the source.

[crazy-max]

@tonistiigi

Did you verify that the changes were only from ignored files? I have a vague memory that it was always the case because of generated code but maybe that has been already fixed. I think in releases we ignore it because of VERSION override.

Yes changes are from the ignored files only:

#16 1.680  D .circleci/config.yml
#16 1.680  D .dockerignore
#16 1.680  D .github/CODEOWNERS
#16 1.680  D .github/ISSUE_TEMPLATE.md
#16 1.680  D .github/PULL_REQUEST_TEMPLATE.md
#16 1.680  D .github/workflows/codeql-analysis.yml
#16 1.680  D .gitignore
#16 1.680  D appveyor.yml
#16 1.680  D cli/winresources/res_windows.go

Generated code is already taken into account and has also been fixed for cli/winresources since #3310.

Also not relevant to this PR, but was going to add BUILDKIT_CONTEXT_KEEP_GIT_DIR=1 in the bake definition to remove the extra git clone step so we can use in the future:

docker buildx bake --set binary.platform=darwin/amd64 "git://github.com/docker/cli#v20.10.10"

@thaJeztah

Can we somehow make the builds work without having to copy the .git directory? It feels like this is hiding the actual issue (i.e., the build now needs the git repository, but effectively just to generate a version)

I think validating the working tree is important as part of the build so we make sure we don't miss something.

[thaJeztah]

I think validating the working tree is important as part of the build so we make sure we don't miss something.

Including .git in the build-context will result in the build-cache being busted even if unrelated changes are made (e.g. creating a new branch, or making changes in another branch will result in changes inside the .git directory). In general, I don't think a build should depend on .git, they're separate concerns.

[tonistiigi]

@thaJeztah If you want to get a cache match for the go build so that it only depends on go sources you can add a filter step in Dockerfile. In practice, I'm not sure if it is beneficial as that step is cached with cache mount already for incremental builds. As far as the build is concerned, all git files are part of the build as their state gets reported in the version output so the logic is correct.

[thaJeztah]

let's get this one in, as it's not changing things w.r.t. what we already do (but possibly we should discuss some)

LGTM