Refactor actions.Client with options to help extensibility#2193
Conversation
rentziass
requested review from
a team,
mumoshu,
nikola-jokic and
toast-gear
as code owners
| assert.Equal(t, want, got) | ||
| }) | ||
|
|
||
| t.Run("Default retries on server error", func(t *testing.T) { |
There was a problem hiding this comment.
Since the HTTP client is now persisted across requests, should we test that the client itself has retries or do we need to keep testing retries on all calls?
There was a problem hiding this comment.
I'd prefer the latter because we can ensure all calls use the retryable client or its equivalent!
There was a problem hiding this comment.
But given that the HTTP client is now re-used for the whole lifetime of an actions.Client wouldn't it make sense to have tests that verify that the option of having (or not) retries is observed? The only way to change the HTTP client down the line is for callers of this package to manually change client.Client, but then I would argue it's that caller to test responsibility that their override is working as intended. What do you think?
There was a problem hiding this comment.
Merging now to unlock other work on expanding the client, but definitely interested in this conversation!
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello! Thank you for your contribution.
Please review our contribution guidelines to understand the project's testing and code conventions.
|
|
||
| retryClient := retryablehttp.NewClient() | ||
|
|
||
| // TODO: this silences retryclient default logger, do we want to provide one |
There was a problem hiding this comment.
It would be great if we could selectively enable the logging, for example, let retryablehttp log requests only when the log level is DEBUG or finer.
That way I'd be able to set the log level to DEBUG when I'm curious why job run delays are unusually long and the retrylablehttp logs would reveal that it's due to excessive retries caused by e.g. an actions service incident.
There was a problem hiding this comment.
Anyway, we'd better address that in another pull request, if necessary, to avoid scope creeping this one!
There was a problem hiding this comment.
I think we might want to have some logging on error cases, especially to log out some HTTP headers so we can check telemetry on the GitHub service.
Ex: x-github-activity-id, ActivityId, etc.
There was a problem hiding this comment.
again, it doesn't have to be this PR. 😄
There was a problem hiding this comment.
Thanks both, I'll look at this separately. It's a bit of a pain that retryablehttp wants a different interface for logging, it'd be great not to have to care about it 😄
mumoshu
left a comment
mumoshu
left a comment
There was a problem hiding this comment.
LGTM. Awesome job @rentziass!!
| return createdRunnerScaleSet, nil | ||
| } | ||
|
|
||
| func (c *Client) UpdateRunnerScaleSet(ctx context.Context, runnerScaleSetId int, runnerScaleSet *RunnerScaleSet) (*RunnerScaleSet, error) { |
There was a problem hiding this comment.
Do we not using UpdateRunnerScaleSet anywhere? 😄
There was a problem hiding this comment.
Yeah I was very surprised, but the only use of it was in its own test 😄
nikola-jokic
left a comment
nikola-jokic
left a comment
There was a problem hiding this comment.
Looks great to me!
4 participants
This is a refactor of
actions.Clientto help extensibility for the upcoming work for supporting proxies and custom TLS. Sorry for the big diff, most of it is test changes to adopt the new API. The notable changes are:NewClientsignature to only require the bare minimum aClientneeds to function correctly, and move every other kind of configuration into aClientOption. This is both to help ease extending the client and to make it easier to rely onNewClientwithin tests: the more logic we add to a client, the more manually instantiating one becomes error prone.NewClient,newActionsServeris now available in tests.UpdateRunnerScaleSetandUpdateRunnerScaleSetmethods were removed, I couldn't find any uses of these?