-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add retry on 401 and 403 for runner-registration #3377
Conversation
77d4eba
to
d8c3355
Compare
github/actions/client.go
Outdated
if retry > 3 { | ||
return nil, fmt.Errorf("unable to register runner after 3 retries: %v", err) | ||
} | ||
time.Sleep(1 * time.Second) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we use a growing interval here? e.g. 250 * time.Millisecond * (retries + 1)
or something like that?
github/actions/client.go
Outdated
var actionsServiceAdminConnection *ActionsServiceAdminConnection | ||
if err := json.NewDecoder(resp.Body).Decode(&actionsServiceAdminConnection); err != nil { | ||
return nil, err | ||
} | ||
|
||
return actionsServiceAdminConnection, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move handling the happy path out of the loop? I think the goal of the loop should be to handle retryable errors?
} | ||
defer resp.Body.Close() | ||
retry := 0 | ||
for { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be painful because this is unexported, but can we add a test for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added, good call!
Co-authored-by: Francesco Renzi <rentziass@gmail.com>
No description provided.