Implement a rate limiting stategy

[svanharmelen]

This adds both a rate limiter in order to try and prevent hitting the
limit all together, and in addition it adds some retry logic.

Fixes #630

[veverkap]

This is awesome!

[svanharmelen]

@veverkap it would be super nice if you could verify this solution?

I have limited access to a GitLab instance and don't actually use GitLab much these days, so I'm going to wait with merging this one until 2 individuals have confirmed that this change doesn't break anything for them and that the rate limiting and retry logic work as expected.

So if you can give the code in this PR a go, that would be very much appreciated!

[veverkap]

Went ahead and tried this locally and couldn't break it.

👍

[svanharmelen]

Thanks for testing it @veverkap! If anyone else can find some time to also test it, then I will merge it right after that... Thanks again!

[totobest]

Tested with upstream project terraform-gitlab-provider and did not break!

[svanharmelen]

Cool... In that case I am going to merge this one today. Thanks for testing it!

[mvisonneau]

I reckon it would be nice to be able to configure this behaviour, the currently hard coded solution returns non 2xx responses in ~5 minutes. This might not be ideal for everyone.

[svanharmelen]

Care to share some more details about that comment @mvisonneau? As that would be a bug in my opinion, so wondering what you observed...

[mvisonneau]

This is for instance what I experience in this test : https://github.com/mvisonneau/gitlab-ci-pipelines-exporter/blob/master/cmd/gitlab_test.go#L180

I am not convinced that I always want to be stuck in a retry-loop for 5m if the GitLab API returns me either a 429 or a 5xx 🤔

[svanharmelen]

It should be simple enough to put in a global switch to disable the retry logic. Would that solve your worries?

Hmm... Actually have a better idea to make this configurable using options when creating the client. Let me wipe up a PR (next week probably) and see if we can find a nice solution.

[mvisonneau]

Yeah I really like the idea of having this retry capability as well and thanks for the implementation btw 🙇. I believe that being able to customize the behaviour at the client config level would be great indeed 👍

[svanharmelen]

So... This turned out to be a backwards incompatible change, but considering most (if not all) people only create 1 client in their code it's a super small change to make your code work with this new logic.

The big advantage is that now we introduced client options in this way, we should be pretty future proof and flexible going forward.

The PR (#813) is pretty big, but the things that matter are:

• https://github.com/xanzy/go-gitlab/pull/813/files#diff-3d26bb4c03811c684c3ebb6529e0e5c0R441-R449
• https://github.com/xanzy/go-gitlab/pull/813/files#diff-3d26bb4c03811c684c3ebb6529e0e5c0R992-R1025
• https://github.com/xanzy/go-gitlab/pull/813/files#diff-1fbdc3985bd3ab083ee75acacf9cc09bR23-R37

In your code this should be a working solution:

func getMockedGitlabClient() (*http.ServeMux, *httptest.Server, *Client) {
	mux := http.NewServeMux()
	server := httptest.NewServer(mux)

	c := &Client{
		Client:      gitlab.NewClient(&http.Client{}, ""),
		RateLimiter: ratelimit.New(100),
	}
	if err := gitlab.WithBaseURL(server.URL)(c); err != nil {
		panic(err) // Or adapt the helper function to return an error
	}
	if err := gitlab.WithoutRetries()(c); err != nil {
		panic(err) // Or adapt the helper function to return an error
	}

	return mux, server, c
}

Would this be a workable solution for you @mvisonneau?

[mvisonneau]

That would be perfectly fine for me, thanks a lot @svanharmelen! 🙇 🙏