fix: fetch revision only then fallback to default refspec #5605

yujunz · 2021-02-25T08:36:19Z

yujunz · 2021-02-25T08:41:52Z

Error messages

 time="2021-02-25T08:10:01Z" level=info msg=Trace args="[git fetch origin --tags --force]" dir="/tmp/argocd@****" operation_name="exec git" time_ms=9547.973753
 time="2021-02-25T08:10:01Z" level=info msg="git checkout --force 45a223fedbb93eeae70820d534257f90a85f961c" dir="/tmp/argocd@****" execID=QfaG4
 time="2021-02-25T08:10:01Z" level=error msg="`git checkout --force 45a223fedbb93eeae70820d534257f90a85f961c` failed exit status 128: fatal: reference is not a tree: 45a223fedbb93e

yujunz · 2021-02-25T08:58:33Z

If performance is a concern, we can add an explicit setting for each repository.

yujunz · 2021-03-03T12:45:16Z

Any comments? @alexmt

codecov · 2021-05-23T11:01:07Z

Codecov Report

Merging #5605 (79998dc) into master (f387ab8) will decrease coverage by 0.01%.
The diff coverage is 20.00%.

@@            Coverage Diff             @@
##           master    #5605      +/-   ##
==========================================
- Coverage   41.93%   41.91%   -0.02%     
==========================================
  Files         176      176              
  Lines       22992    22992              
==========================================
- Hits         9641     9637       -4     
- Misses      11966    11972       +6     
+ Partials     1385     1383       -2

Impacted Files	Coverage Δ
reposerver/repository/repository.go	`57.64% <20.00%> (-0.45%)`	⬇️

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 f387ab8...79998dc. Read the comment docs.

alexmt · 2022-01-04T02:19:40Z

Sorry, PR slip off radar.

snuggie12 · 2022-01-21T01:04:54Z

@alexmt apologies if a faux pas, but do you have an ETA on this one? Just wanted to make sure this didn't fall through the cracks again due to the holidays.

yujunz · 2022-01-24T05:38:43Z

Flake? The failure seems not related to the change.

2022-01-24T03:00:04.1594119Z time="2022-01-24T03:00:04Z" level=debug msg="Application 'test-hook-before-hook-creation-failure' operation terminating\n" duration=204.602806ms execID=47273
2022-01-24T03:00:04.1595605Z time="2022-01-24T03:00:04Z" level=info msg="expectation succeeded: no error and output contained ''"
2022-01-24T03:00:04.1659029Z time="2022-01-24T03:00:04Z" level=info msg="pending: operation phase should be Failed, is Running"
2022-01-24T03:00:07.1712699Z time="2022-01-24T03:00:07Z" level=info msg="pending: operation phase should be Failed, is Succeeded"
2022-01-24T03:00:10.1777595Z time="2022-01-24T03:00:10Z" level=info msg="pending: operation phase should be Failed, is Succeeded"
2022-01-24T03:00:13.1833939Z time="2022-01-24T03:00:13Z" level=info msg="pending: operation phase should be Failed, is Succeeded"
2022-01-24T03:00:16.1909338Z time="2022-01-24T03:00:16Z" level=info msg="pending: operation phase should be Failed, is Succeeded"
2022-01-24T03:00:19.1918959Z ##[error]    hook_test.go:351: timeout waiting for: operation phase should be Failed, is Succeeded
2022-01-24T03:00:19.1920454Z --- FAIL: TestHookBeforeHookCreationFailure (18.96s)
2022-01-24T03:00:19.1920913Z FAIL
2022-01-24T03:00:19.1983630Z FAIL	github.com/argoproj/argo-cd/v2/test/e2e	651.703s
2022-01-24T03:00:19.2048239Z FAIL

yujunz · 2022-01-25T08:10:28Z

Failed on the same case

Error:     hook_test.go:351: timeout waiting for: operation phase should be Failed, is Succeeded
--- FAIL: TestHookBeforeHookCreationFailure (18.93s)
FAIL
FAIL	github.com/argoproj/argo-cd/v2/test/e2e	643.169s
FAIL

Is the master branch broken?

alexmt · 2022-01-27T18:29:55Z

reposerver/repository/repository.go

+	err = gitClient.Fetch(revision)
+
+	if err != nil {
+		log.Warnf("Failed to fetch revision %s: %v", revision, err)


Please change it to log.Info. We are trying to use 'warning' logs only for something that the operator should act on.

alexmt · 2022-01-27T18:31:28Z

util/git/client.go

@@ -335,7 +335,7 @@ func (m *nativeGitClient) Fetch(revision string) error {

 	var err error
 	if revision != "" {
-		err = m.runCredentialedCmd("git", "fetch", "origin", revision, "--tags", "--force")
+		err = m.runCredentialedCmd("git", "fetch", "origin", revision)


Can you please clarify why this change is necessary? Can we rollback this line please unless it is really needed?

No need to fetch tags on explicit revision. This reduce a bit the pack size on large repository with many tags.

Feel free to revert the change though.

Sounds good. Reverted this change. The rest looks good! Merging once CI is green.

Ignoring commit SHA breaks gerrit when the commit is not merged Signed-off-by: Yujun Zhang <yujunz@nvidia.com>

Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com>

) * fix: fallback to fetch default only on error Ignoring commit SHA breaks gerrit when the commit is not merged Signed-off-by: Yujun Zhang <yujunz@nvidia.com> * revert util/git/client.go changes Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com> Co-authored-by: Alexander Matyushentsev <AMatyushentsev@gmail.com> Signed-off-by: pashavictorovich <pavel@codefresh.io>

crenshaw-dev · 2022-03-24T18:53:58Z

If performance is a concern, we can add an explicit setting for each repository.

@yujunz can you expand on this? I'm observing performance issues (lots of large packfiles) for some repos. But I don't know enough about git to understand why this happens.

yujunz · 2022-03-25T09:05:49Z

Not sure what kind of pack files from your repository. This patch mainly reduces the total refs to fetch. It is not helping when the repository has many large files or long commit history.

yujunz force-pushed the fix-fetch branch from 393e18d to 53d05aa Compare February 25, 2021 08:36

yujunz marked this pull request as ready for review February 25, 2021 08:58

yujunz mentioned this pull request Mar 23, 2021

Fetch target revision which is not merged to HEAD #1604

Closed

james-callahan mentioned this pull request Apr 8, 2021

Allow referring to non-default remote references #4935

Closed

yujunz force-pushed the fix-fetch branch from 53d05aa to 20d762f Compare May 23, 2021 11:01

yujunz force-pushed the fix-fetch branch from 20d762f to 5b31f58 Compare May 23, 2021 11:02

yujunz changed the title ~~fix: fallback to fetch default only on error (#5320)~~ fix: fetch revision only then fallback to default refspec May 23, 2021

yujunz force-pushed the fix-fetch branch from 5b31f58 to 37bd092 Compare June 3, 2021 11:51

yujunz force-pushed the fix-fetch branch from 37bd092 to 4432ea6 Compare January 4, 2022 02:09

alexmt self-requested a review January 4, 2022 02:21

alexmt self-assigned this Jan 4, 2022

alexmt added this to the v2.3 milestone Jan 21, 2022

yujunz force-pushed the fix-fetch branch from 4432ea6 to a93b373 Compare January 24, 2022 02:38

yujunz force-pushed the fix-fetch branch from a93b373 to a4fefb1 Compare January 25, 2022 07:17

alexmt requested changes Jan 27, 2022

View reviewed changes

fix: fallback to fetch default only on error

b55627a

Ignoring commit SHA breaks gerrit when the commit is not merged Signed-off-by: Yujun Zhang <yujunz@nvidia.com>

yujunz force-pushed the fix-fetch branch from a4fefb1 to b55627a Compare January 28, 2022 07:26

revert util/git/client.go changes

79998dc

Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com>

alexmt approved these changes Jan 28, 2022

View reviewed changes

alexmt merged commit 4aa614d into argoproj:master Jan 28, 2022

yujunz deleted the fix-fetch branch February 4, 2022 09:17

crenshaw-dev mentioned this pull request Mar 23, 2022

repo-server duplicates git packfiles, filling up disk #8845

Closed

yujunz mentioned this pull request Mar 30, 2022

Make git fetch behavior configurable #8938

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: fetch revision only then fallback to default refspec #5605

fix: fetch revision only then fallback to default refspec #5605

yujunz commented Feb 25, 2021 •

edited

Loading

yujunz commented Feb 25, 2021

yujunz commented Feb 25, 2021

yujunz commented Mar 3, 2021

codecov bot commented May 23, 2021 •

edited

Loading

alexmt commented Jan 4, 2022

snuggie12 commented Jan 21, 2022

yujunz commented Jan 24, 2022

yujunz commented Jan 25, 2022

alexmt Jan 27, 2022

yujunz Jan 28, 2022

alexmt Jan 27, 2022

yujunz Jan 28, 2022

alexmt Jan 28, 2022

crenshaw-dev commented Mar 24, 2022

yujunz commented Mar 25, 2022

fix: fetch revision only then fallback to default refspec #5605

fix: fetch revision only then fallback to default refspec #5605

Conversation

yujunz commented Feb 25, 2021 • edited Loading

yujunz commented Feb 25, 2021

yujunz commented Feb 25, 2021

yujunz commented Mar 3, 2021

codecov bot commented May 23, 2021 • edited Loading

Codecov Report

alexmt commented Jan 4, 2022

snuggie12 commented Jan 21, 2022

yujunz commented Jan 24, 2022

yujunz commented Jan 25, 2022

alexmt Jan 27, 2022

Choose a reason for hiding this comment

yujunz Jan 28, 2022

Choose a reason for hiding this comment

alexmt Jan 27, 2022

Choose a reason for hiding this comment

yujunz Jan 28, 2022

Choose a reason for hiding this comment

alexmt Jan 28, 2022

Choose a reason for hiding this comment

crenshaw-dev commented Mar 24, 2022

yujunz commented Mar 25, 2022

yujunz commented Feb 25, 2021 •

edited

Loading

codecov bot commented May 23, 2021 •

edited

Loading