Additional git tagger variants (TreeSha, AbbrevTreeSha)

[corneliusweig]

Synopsis

PR #1902 has introduced git tagger variants Tags, CommitSha, and AbbrevCommitSha. Issue #407 also suggests tree hash variants, which are part of this PR. Originally these variants were also part of #1902, but OSX has problems to determine the relative git path, so these two variants were extracted.

• TreeSha: use the full tree hash of the artifact workingdir
• AbbrevTreeSha: use the abbreviated tree hash of the artifact workingdir

The tree hash variants are similar in spirit to the sha256 tagger in that it is based on the (committed) workingdir content. As the hash is pre-computed by git, this should be faster than the sha256Tagger.

Config change

There are no config changes wrt. PR #1902, but gitCommit.variant now understands the TreeSha and AbbrevTreeSha variants:

tagPolicy:
  gitCommit:
   # variant is an enum with valid values (case-insensitive)
   # - Tags
   # - CommitSha
   # - AbbrevCommitSha
   # - TreeSha                  NEW
   # - AbbrevTreeSha       NEW
    variant: Tags # default

Close #407

[corneliusweig]

So the problem on OSX seems to be that filepath.Abs() reports /var/folders/... for the workingdir, but git rev-parse --show-toplevel prepends an additional folder: /private/var/folders/....

absWd=/var/folders/nz/vv4_9tw56nv9k3tkvyszvwg80000gn/T/skaffold810560671
gitRoot=/private/var/folders/nz/vv4_9tw56nv9k3tkvyszvwg80000gn/T/skaffold810560671

So this is probably a real problem on OSX and not just a bad test setup.

[codecov-io]

Codecov Report

Merging #1905 into master will increase coverage by 0.07%.
The diff coverage is 82.6%.

@@            Coverage Diff             @@
##           master    #1905      +/-   ##
==========================================
+ Coverage   56.74%   56.81%   +0.07%     
==========================================
  Files         183      183              
  Lines        7860     7883      +23     
==========================================
+ Hits         4460     4479      +19     
- Misses       2988     2990       +2     
- Partials      412      414       +2

Impacted Files	Coverage Δ
pkg/skaffold/schema/latest/config.go	100% <ø> (ø)	⬆️
pkg/skaffold/schema/v1beta10/upgrade.go	66.66% <ø> (ø)	⬆️
pkg/skaffold/build/tag/git_commit.go	80.88% <82.6%> (+0.88%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eb9916f...a977f5c. Read the comment docs.

[nkubala]

So the problem on OSX seems to be that filepath.Abs() reports /var/folders/... for the workingdir, but git rev-parse --show-toplevel prepends an additional folder: /private/var/folders/....

I actually don't see this when I run git rev-parse --show-toplevel on my machine (git version 2.19.0.605). might just be a weird quirk of some versions of git. in the case that it fails, is there something we can use as a fallback? maybe try and strip off the /private prefix if we get a failure, and then actually fail if that also doesn't work?

[corneliusweig]

@nkubala So I think the root cause is that /var on some OSX variants is actually a symlink to /private/var. Go Abs does not expand the symlink, whereas git rev-parse --show-toplevel does. At least this is my assumption, because in lack of an OSX system I can't be sure.
However, I would not put in some extra logic for stripping /private, this just feels horribly brittle. For example, if an affected OSX user has another symlink somewhere in his dir hierarchy, this would fail already.
Maybe you can do further checks on your machine?

[tejal29]

approve with nits!

[tejal29]

I only verified this on Linux. Please hold until i verify on mac too.

I only verified this on Linux. Please hold until i verify on mac too.

[dgageot]

@corneliusweig can this be merged?

[corneliusweig]

@dgageot No, it should come after #2109 (config freeze). Unless this change goes under the radar config-wise. All it does is add new enum values, so this would be possible without breaking anybody.

[nkubala]

@corneliusweig I merged the config version upgrade PR, can you add an change to upgrade() in the v1beta10 config? then I'll merge this in

[corneliusweig]

@nkubala Done. I merged in master after the config freeze.