fix: Pick up correct commit SHA when using annotated git tags (#4231) #4538

jaideepr97 · 2020-10-12T16:19:36Z

Checklist:

Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
The title of the PR states what changed and the related issues number (used for the release note).

I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
Does this PR require documentation updates?
I've updated documentation as required by this PR.
Optional. My organization is added to USERS.md.
I've signed the CLA and my build is green (troubleshooting builds).

fixes: #4231
Argocd seems to be picking up the hash of the tag from the output of ls-remote rather than the commit SHA in case of annotated tags, this fix checks to see if the picked up revision matches the commit SHA or not, and replaces the tag hash with the commit SHA if not

CLAassistant · 2020-10-12T16:19:41Z

All committers have signed the CLA.

wtam2018 · 2020-10-14T03:17:55Z

reposerver/repository/repository.go

@@ -163,6 +163,11 @@ func (s *Service) runRepoOperation(
 		if err != nil {
 			return nil, err
 		}
+		sha, err := gitClient.CommitSHA()


I am skeptical about this fix. gitClient.CommitSHA() gives us the SHA of the HEAD which may not be the resolving tag is pointing to. I think we want to figure out what caused SHA resolution hehavior change since v1.4.2.

I just tested it again to be sure, it is picking up the correct commit SHA for the specified tag even when HEAD points to a different SHA
I think it maybe using the state of the commit history from a particular revision so HEAD resolves to the same as the required commit

As for the behaviour change, from what I understand, the lsRemote function just runs git ls-remote's output, which is the list of all refs and tags with their SHAs. In this list, it seems the annotated tag's hash always appears before the SHA of the commit it points to (which is in the next line below it) and the current code just traverses the list and picks up the first one it sees, and ends up taking the hash of the tag instead of the commit SHA

if I had to guess, maybe the commit SHA showed up before the tag hash at some point and the ordering of the hashes present in the output of ls-remote may have changed recently (since the argocd code for this doesn't seem to have changed between 1.4.2 and the current version)

IMHO, the only way to get the actual revision of the commit an annotated tag is pointing to is to checkout the tag, bringing the repository into detached HEAD state, and then resolve HEAD revision.

The checkout to the target revision is done a little later down the code path (

argo-cd/reposerver/repository/repository.go

Line 213 in 3ae1d13

_, err = checkoutRevision(gitClient, revision, log.WithField("repo", repo.Repo))

). So probably the current incarnation of this PR will only work if the annotated tag is actually pointing to the real HEAD of the repository's default branch.

@jaanfis
The fix is working even when HEAD points to a different commit SHA
Seeing some strange behavior I haven't been able to explain yet though, CommitSHA() seems to be running git rev-parse HEAD but returning the correct commit SHA almost as if the correct revision of the repo is somehow already being checked out even before the actual call to checkoutRevision happens, which happens later like you pointed out
Even when immediately before/after HEAD does get resolved to the actual HEAD in a different function call
There seems to be some kind of inconsistency between the api-server and repo-server here as to what HEAD actually points to

22:09:32 repo-server | DEBU[0039] revision 'v0.0.8' resolved to '95249be61b028d566c29d47b19e65c5603388a41' 22:09:32 repo-server | INFO[0039] git fetch origin --tags --force dir="/tmp/https:__github.com_jaideepr97_argocd-experiment" execID=OAWIY 22:09:32 repo-server | DEBU[0039] revision 'v0.0.8' resolved to '95249be61b028d566c29d47b19e65c5603388a41' 22:09:32 repo-server | INFO[0039] git rev-parse HEAD dir="/tmp/https:__github.com_jaideepr97_argocd-experiment" execID=W212M 22:09:32 repo-server | DEBU[0039] 632039659e542ed7de0c170a4fcc1c571b288fc0 duration=3.83898ms execID=W212M 22:09:32 repo-server | INFO[0039] app details cache miss: 632039659e542ed7de0c170a4fcc1c571b288fc0/&ApplicationSource{RepoURL:https://github.com/jaideepr97/ARGOCD-EXPERIMENT,Path:helm-guestbook,TargetRevision:v0.0.8,Helm:nil,Kustomize:nil,Ksonnet:nil,Directory:nil,Plugin:nil,Chart:,} 22:09:32 controller | INFO[0040] Successfully saved info of 1 clusters 22:09:32 api-server | 2020/10/15 22:09:32 proto: tag has too few fields: "-" 22:09:32 api-server | INFO[0039] received unary call /application.ApplicationService/Create grpc.method=Create grpc.request.claims=null grpc.request.content="%!v(PANIC=String method: reflect.Value.Bytes of non-byte slice)" grpc.service=application.ApplicationService grpc.start_time="2020-10-15T22:09:32Z" span.kind=server system=grpc 22:09:33 api-server | DEBU[0040] symbolic reference 'HEAD' (refs/heads/master) resolved to '401ea4c66bbd95c18eb3ed4d05912514cae83f30'

Here 401ea4c66bbd95c18eb3ed4d05912514cae83f30 is the actual HEAD, 95249be61b028d566c29d47b19e65c5603388a41 is the hash of the annotated tag and 632039659e542ed7de0c170a4fcc1c571b288fc0 is the commit I want

@jaideepr97
It is a regression introduced in v1.6.0.
The revision returned from checkoutRevision() was ignored. This should fix it.
wtam2018@3b61354

@alexmt Do you recall the reason to ignore the revision returned from checkoutRevision()?
4f49168

Thank you for finding the root cause @wtam2018 , @jaideepr97 . Finally found why changes 4f49168 was implemented.
That was an attempt to fix caching for git tags. The problem is that we were using tag hash, returned by ls-remote to load cached manifests:

argo-cd/reposerver/repository/repository.go

Line 169 in 15b0a6e

result, obj, err := getCached(revision, true)

and then commit sha to store generated manifests in cache:

argo-cd/reposerver/repository/repository.go

Line 227 in 15b0a6e

return operation(appPath, gitClient.Root(), revision, signature)

(operation eventually uses revision to store cache).

So repo-server was unable to property cache manifests for annotated tags and had to regenerate them again and again.

I think this PR re-introduced old bug again. @jaideepr97 I think we can fix it by adding one more parameter to operation that holds ls-remote result, so that operation has both commitSHA and revision used to retrieve cache.

…ocd-#4231

codecov-io · 2020-10-19T15:08:41Z

Codecov Report

Merging #4538 into master will increase coverage by 0.02%.
The diff coverage is 84.61%.

@@            Coverage Diff             @@
##           master    #4538      +/-   ##
==========================================
+ Coverage   41.31%   41.34%   +0.02%     
==========================================
  Files         124      124              
  Lines       16976    16976              
==========================================
+ Hits         7014     7018       +4     
+ Misses       8950     8948       -2     
+ Partials     1012     1010       -2

Impacted Files	Coverage Δ
reposerver/repository/repository.go	`60.61% <84.61%> (+0.61%)`	⬆️

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 cf03c1d...155eecf. Read the comment docs.

…ocd-#4231

…eing used to store and retrieve manifests in cache

wtam2018 · 2020-10-21T18:43:25Z

reposerver/repository/repository.go

@@ -145,7 +145,7 @@ func (s *Service) runRepoOperation(
 	source *v1alpha1.ApplicationSource,
 	verifyCommit bool,
 	getCached func(revision string, firstInvocation bool) (bool, interface{}, error),
-	operation func(appPath, repoRoot, revision, verifyResult string) (interface{}, error),
+	operation func(appPath, repoRoot, commitSHA, revision, verifyResult string) (interface{}, error),


We might want to add some commment to describe the difference between these parameters. commitSHA is a resolved SHA from a revision. It goes to the generated manifest. revision is used as a key to manifest cache. I would probably rename revision to cacheKey.

…ocd-#4231

…for cache usage, renamed revision to cacheKey

wtam2018 · 2020-10-22T18:58:34Z

LGTM
@jaideepr97 thanks for the fix.

jaideepr97 · 2020-10-22T19:52:46Z

LGTM
@jaideepr97 thanks for the fix.

Thanks for the help and feedback!

alexmt

LGTM. Thank you @jaideepr97 !

jaideepr97 added 7 commits October 7, 2020 12:36

fixed wrong sha for annotated git tags

c8c8c6c

rebased

47d4ae5

rebased against master

145c612

restructured tests

9db0d85

fixing make test fail

ea003c4

deleted commented out code

218c2ea

reverted makefile change on L15

bae38a7

jessesuen requested a review from alexmt October 13, 2020 21:12

jessesuen assigned alexmt Oct 13, 2020

wtam2018 reviewed Oct 14, 2020

View reviewed changes

jaideepr97 added 6 commits October 14, 2020 10:57

Merge branch 'master' of https://github.com/argoproj/argo-cd into arg…

1293e74

…ocd-#4231

debug print statements

f4a8c16

Merge branch 'master' of https://github.com/argoproj/argo-cd into arg…

a423b5e

…ocd-#4231

updated fix to not ignore output of checkoutRevision

b0535a7

fixed ineffectual assignment of variable in test

4aa0852

Merge branch 'master' of https://github.com/argoproj/argo-cd into arg…

4fd930a

…ocd-#4231

jaideepr97 added 3 commits October 21, 2020 09:01

Merge branch 'master' of https://github.com/argoproj/argo-cd into arg…

7becdae

…ocd-#4231

Merge branch 'master' of https://github.com/argoproj/argo-cd into arg…

41780fc

…ocd-#4231

incorporated feedback to account for different meanings of revision b…

930b435

…eing used to store and retrieve manifests in cache

wtam2018 reviewed Oct 21, 2020

View reviewed changes

jaideepr97 added 2 commits October 22, 2020 09:01

Merge branch 'master' of https://github.com/argoproj/argo-cd into arg…

9af0dcd

…ocd-#4231

added comment to highlight difference between commitSHA and revision …

155eecf

…for cache usage, renamed revision to cacheKey

alexmt approved these changes Oct 22, 2020

View reviewed changes

alexmt merged commit bdee71d into argoproj:master Oct 22, 2020

aenshin-pp mentioned this pull request Sep 25, 2024

Argo CD start to show annotated tag's hash instead of commit's hash after updating to new version #20082

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Pick up correct commit SHA when using annotated git tags (#4231) #4538

fix: Pick up correct commit SHA when using annotated git tags (#4231) #4538

jaideepr97 commented Oct 12, 2020 •

edited

Loading

CLAassistant commented Oct 12, 2020 •

edited

Loading

wtam2018 Oct 14, 2020 •

edited

Loading

jaideepr97 Oct 14, 2020 •

edited

Loading

jannfis Oct 14, 2020

jaideepr97 Oct 15, 2020 •

edited

Loading

wtam2018 Oct 16, 2020 •

edited

Loading

wtam2018 Oct 16, 2020

alexmt Oct 19, 2020

codecov-io commented Oct 19, 2020 •

edited

Loading

wtam2018 Oct 21, 2020

wtam2018 commented Oct 22, 2020

jaideepr97 commented Oct 22, 2020

alexmt left a comment

fix: Pick up correct commit SHA when using annotated git tags (#4231) #4538

fix: Pick up correct commit SHA when using annotated git tags (#4231) #4538

Conversation

jaideepr97 commented Oct 12, 2020 • edited Loading

CLAassistant commented Oct 12, 2020 • edited Loading

wtam2018 Oct 14, 2020 • edited Loading

Choose a reason for hiding this comment

jaideepr97 Oct 14, 2020 • edited Loading

Choose a reason for hiding this comment

jannfis Oct 14, 2020

Choose a reason for hiding this comment

jaideepr97 Oct 15, 2020 • edited Loading

Choose a reason for hiding this comment

wtam2018 Oct 16, 2020 • edited Loading

Choose a reason for hiding this comment

wtam2018 Oct 16, 2020

Choose a reason for hiding this comment

alexmt Oct 19, 2020

Choose a reason for hiding this comment

codecov-io commented Oct 19, 2020 • edited Loading

Codecov Report

wtam2018 Oct 21, 2020

Choose a reason for hiding this comment

wtam2018 commented Oct 22, 2020

jaideepr97 commented Oct 22, 2020

alexmt left a comment

Choose a reason for hiding this comment

jaideepr97 commented Oct 12, 2020 •

edited

Loading

CLAassistant commented Oct 12, 2020 •

edited

Loading

wtam2018 Oct 14, 2020 •

edited

Loading

jaideepr97 Oct 14, 2020 •

edited

Loading

jaideepr97 Oct 15, 2020 •

edited

Loading

wtam2018 Oct 16, 2020 •

edited

Loading

codecov-io commented Oct 19, 2020 •

edited

Loading