Retry `git clone` on git clone failure in `gh repo fork --clone` by jsoref · Pull Request #6962 · cli/cli

jsoref · 2023-02-03T19:46:52Z

Fixes #5118 -- hopefully

This is entirely untested, but the linter likes it, so there's that.

samcoe

@jsoref Thanks for starting on this work. It looks good so far. I left a comment regarding keeping the retry mechanism internal and not exposing those low level implementation details to our users. Let me know what you think.

Additionally, if we could get a test verifying this retry behavior works I think this is good to ship.

pkg/cmd/repo/clone/clone.go

jsoref · 2023-02-07T02:08:41Z

I'm happy to drop the flags. I have no idea how to test the failure case.

samcoe · 2023-02-07T02:38:52Z

@jsoref No problem. Feel free to commit code for dropping of the flags and one of our team members will take over writing of a test.

jsoref · 2023-02-07T02:50:17Z

Great. I couldn't even get it to build. The Makefile wasn't helpful; I ran into the incredibly confounding:

$ go mod download
go: modules disabled by GO111MODULE=off; see 'go help modules'

(Which does not explain anything relating to the message.)

And even when I forced it to on, nothing happened.

mislav · 2023-02-07T14:21:25Z

I ran into the incredibly confounding:

It might help you to remove the legacy GO111MODULE environment variable from wherever it's being set.

GO111MODULE= make

Our project builds in a pristine Go development environment, but we can't guarantee building in an environment you have modified.

jsoref · 2023-02-07T14:26:56Z

That didn't work at all (I've tried repeatedly while fighting various golang projects), apparently golang hid its setting in some config file, so I wanted go env -w GO111MODULE=... (I may file a PR to improve the messaging, as it's really unhelpful). But even once I did that, I didn't actually get any dependencies. Yes, I have a build env that is used to develop lots of things and has evolved over ~6 years.

mislav · 2023-02-07T14:32:17Z

Maybe then explicitly setting it to "on" just for the gh project might be the way to go? GO111MODULE=on go build -o bin/gh ./cmd/gh

jsoref · 2023-02-07T14:56:06Z

That worked. Now I have a gh and the run test and debug test links in VSCode work...

mislav

Thanks for taking this on! My main concern is that this doesn't solve your original problem (more about that in comments). My secondary concerns are that it would be best that the retry code is testable and that it supports Context cancellation.

pkg/cmd/repo/clone/clone.go

mislav · 2023-02-07T15:45:42Z

pkg/cmd/repo/clone/clone.go

+		if err == nil {
+			break
+		}
+		time.Sleep(retryBackoff * (time.Duration(i) + 1))


Testable Go code should never be allowed to sleep the same period of time in tests as it does during normal runtime. Even a relatively short duration like sleeping 100ms would make tests really slow. Finding a way to inject a shorter (fake) Duration in tests would be crucial.

Also, the Sleep statement would not return early if a Context has been cancelled, therefore if a Context is involved, it would be better to use a Timer and use a select statement to react to either a Timer completing or a Context being cancelled, whichever comes first. Example: https://github.com/cli/oauth/blob/adf5f733ffd14993aad9f3ff02c757a737f7c63b/device/poller.go#L30-L37

This is beyond what I'm up for doing at this time.

It should involve introducing a poller.go, and really there should be a poller_test.go -- and even that repo doesn't have one.

pkg/cmd/repo/clone/clone.go

samcoe · 2023-02-08T00:20:58Z

@jsoref @mislav I went ahead and updated this PR to use a new version of the BackOff package which helped clean things up a lot. I also wrote a couple tests. Let me know what you think. Only thing I was unsure of was the exponential backoff duration variables. Not sure we really need exponential backoff here and we could simplify to just a set backoff duration.

The BackOff package is fairly easy to use and well documented, I would vote for using it going forward in regards to #6985.

I did have to rebase the PR to fix the tests I wrote that were failing due the recent remote add changes.

samcoe · 2023-02-08T00:27:42Z

pkg/cmd/repo/fork/fork_test.go

-		ios.SetStdoutTTY(tt.tty)
-		ios.SetStderrTTY(tt.tty)
-		tt.opts.IO = ios
+		t.Run(tt.name, func(t *testing.T) {


Something that I had not thought about until now is that when using the -run flag with go test that all this set up happens for each test even though the tests won't be run. If we encapsulate all the code in t.Run the setup won't happen unless the test is actually going to be run.

jsoref · 2023-02-08T00:48:01Z

pkg/cmd/repo/fork/fork.go

+			cloneDir, err := backoff.RetryWithData(func() (string, error) {
+				forkedRepoURL := ghrepo.FormatRemoteURL(forkedRepo, protocol)
+				dir, err := gitClient.Clone(ctx, forkedRepoURL, opts.GitArgs)
+				if err == nil {
+					return dir, err
+				}
+				var execError errWithExitCode
+				if errors.As(err, &execError) && execError.ExitCode() == 128 {
+					return "", err
+				}
+				return "", backoff.Permanent(err)
+			}, backoff.WithContext(backoff.WithMaxRetries(opts.BackOff, 3), ctx))


Will users see an indication of the failures/retries?

Since I trigger this problem regularly, I'd love to have a sense of how many retries I'm spending.

As of right now, no they will not. Could be added in though.

I think it's worth adding. That'd help inform decisions about the right time values.

Alternatively, I suppose I could try using this version of gh w/ that logging and see what they say...

Lemme see if I can manage to make that work locally...

jsoref · 2023-02-08T00:49:48Z

pkg/cmd/repo/fork/fork.go

+				bo.Multiplier = 1.1
+				bo.MaxInterval = 10 * time.Second
+				bo.MaxElapsedTime = 5 * time.Minute


I have absolutely no idea what proper numbers would be. I stole mine from something else because it was suggested.

IME, a clone very shortly after the failed clone works, but I don't have a great sense of scale and I'm not sure if it relates to the size of a repository (and if that's by commits, by number of files in the working directory, or some other measure).

My clones come from repositories of various sizes somewhat randomly/haphazardly...

Addressed comments and had offline conversation about changes.

jsoref requested a review from a team as a code owner February 3, 2023 19:46

jsoref requested review from mislav and removed request for a team February 3, 2023 19:46

cliAutomation added the external pull request originating outside of the CLI core team label Feb 3, 2023

samcoe reviewed Feb 7, 2023

View reviewed changes

pkg/cmd/repo/clone/clone.go Outdated Show resolved Hide resolved

samcoe self-assigned this Feb 7, 2023

jsoref force-pushed the issue-5118 branch from 31fffc2 to 5f43df3 Compare February 7, 2023 02:48

mislav previously requested changes Feb 7, 2023

View reviewed changes

jsoref mentioned this pull request Feb 7, 2023

Unify backoff retry mechanisms #6985

Closed

jsoref changed the title ~~Retry git clone~~ Retry git clone on git clone failure in gh repo fork --clone Feb 7, 2023

jsoref force-pushed the issue-5118 branch from 5f43df3 to 430270e Compare February 7, 2023 18:33

jsoref requested a review from mislav February 7, 2023 18:37

jsoref and others added 3 commits February 8, 2023 11:22

Retry git clone when gh repo fork --clone fails

5e7a0ea

Address PR comments

18284b4

Fix up tests

0c343b5

samcoe force-pushed the issue-5118 branch from 3feaa9a to 0c343b5 Compare February 8, 2023 00:23

samcoe self-requested a review February 8, 2023 00:24

samcoe reviewed Feb 8, 2023

View reviewed changes

jsoref commented Feb 8, 2023

View reviewed changes

Use constant backoff instead of exponential

c6ace04

samcoe approved these changes Feb 21, 2023

View reviewed changes

samcoe enabled auto-merge (squash) February 21, 2023 03:59

samcoe merged commit 7541ee6 into cli:trunk Feb 21, 2023

jsoref deleted the issue-5118 branch February 21, 2023 04:27

Conversation

jsoref commented Feb 3, 2023

Uh oh!

samcoe left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jsoref commented Feb 7, 2023

Uh oh!

samcoe commented Feb 7, 2023

Uh oh!

jsoref commented Feb 7, 2023

Uh oh!

mislav commented Feb 7, 2023

Uh oh!

jsoref commented Feb 7, 2023

Uh oh!

mislav commented Feb 7, 2023

Uh oh!

jsoref commented Feb 7, 2023

Uh oh!

mislav left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mislav Feb 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsoref Feb 7, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

samcoe commented Feb 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

samcoe Feb 8, 2023

Choose a reason for hiding this comment

Uh oh!

jsoref Feb 8, 2023

Choose a reason for hiding this comment

Uh oh!

samcoe Feb 8, 2023

Choose a reason for hiding this comment

Uh oh!

jsoref Feb 8, 2023

Choose a reason for hiding this comment

Uh oh!

jsoref Feb 8, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mislav Feb 7, 2023 •

edited

Loading

samcoe commented Feb 8, 2023 •

edited

Loading