-
Notifications
You must be signed in to change notification settings - Fork 28
feat: add missing htt StatusCode and make it settable #6051
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
base: main
Are you sure you want to change the base?
Changes from all commits
09a7868
6ef6083
96b5e2a
35acc27
a63c84c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1 @@ | ||
| v0.25.0 | ||
| v0.26.0 |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -10,7 +10,7 @@ import ( | |||||
| "github.com/stackitcloud/stackit-sdk-go/core/utils" | ||||||
| ) | ||||||
|
|
||||||
| var RetryHttpErrorStatusCodes = []int{http.StatusBadGateway, http.StatusGatewayTimeout} | ||||||
| var RetryHttpErrorStatusCodes = []int{http.StatusBadGateway, http.StatusGatewayTimeout, http.StatusServiceUnavailable} | ||||||
|
|
||||||
| // AsyncActionCheck reports whether a specific async action has finished. | ||||||
| // - waitFinished == true if the async action is finished, false otherwise. | ||||||
|
|
@@ -20,25 +20,33 @@ type AsyncActionCheck[T any] func() (waitFinished bool, response *T, err error) | |||||
|
|
||||||
| // AsyncActionHandler handles waiting for a specific async action to be finished. | ||||||
| type AsyncActionHandler[T any] struct { | ||||||
| checkFn AsyncActionCheck[T] | ||||||
| sleepBeforeWait time.Duration | ||||||
| throttle time.Duration | ||||||
| timeout time.Duration | ||||||
| tempErrRetryLimit int | ||||||
| IntermediateStateReached bool | ||||||
| checkFn AsyncActionCheck[T] | ||||||
| sleepBeforeWait time.Duration | ||||||
| throttle time.Duration | ||||||
| timeout time.Duration | ||||||
| tempErrRetryLimit int | ||||||
| IntermediateStateReached bool | ||||||
| retryHttpErrorStatusCodes []int | ||||||
| } | ||||||
|
|
||||||
| // New initializes an AsyncActionHandler | ||||||
| func New[T any](f AsyncActionCheck[T]) *AsyncActionHandler[T] { | ||||||
| return &AsyncActionHandler[T]{ | ||||||
| checkFn: f, | ||||||
| sleepBeforeWait: 0 * time.Second, | ||||||
| throttle: 5 * time.Second, | ||||||
| timeout: 30 * time.Minute, | ||||||
| tempErrRetryLimit: 5, | ||||||
| checkFn: f, | ||||||
| sleepBeforeWait: 0 * time.Second, | ||||||
| throttle: 5 * time.Second, | ||||||
| timeout: 30 * time.Minute, | ||||||
| tempErrRetryLimit: 5, | ||||||
| retryHttpErrorStatusCodes: RetryHttpErrorStatusCodes, | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| // SetRetryHttpErrorStatusCodes sets the retry codes to use for retry. | ||||||
| func (h *AsyncActionHandler[T]) SetRetryHttpErrorStatusCodes(c []int) *AsyncActionHandler[T] { | ||||||
| h.retryHttpErrorStatusCodes = c | ||||||
| return h | ||||||
| } | ||||||
|
|
||||||
| // SetThrottle sets the time interval between each check of the async action. | ||||||
| func (h *AsyncActionHandler[T]) SetThrottle(d time.Duration) *AsyncActionHandler[T] { | ||||||
| h.throttle = d | ||||||
|
|
@@ -122,7 +130,7 @@ func (h *AsyncActionHandler[T]) handleError(retryTempErrorCounter int, err error | |||||
| return retryTempErrorCounter, fmt.Errorf("found non-GenericOpenApiError: %w", err) | ||||||
| } | ||||||
| // Some APIs may return temporary errors and the request should be retried | ||||||
| if !utils.Contains(RetryHttpErrorStatusCodes, oapiErr.StatusCode) { | ||||||
| if !utils.Contains(h.retryHttpErrorStatusCodes, oapiErr.StatusCode) { | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
I'd prefer the builtin func instead of the utils one, if we are already touching this line
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We'd also need a fallback here when |
||||||
| return retryTempErrorCounter, err | ||||||
| } | ||||||
| retryTempErrorCounter++ | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,11 +23,12 @@ func TestNew(t *testing.T) { | |
| checkFn := func() (waitFinished bool, res *interface{}, err error) { return true, nil, nil } | ||
| got := New(checkFn) | ||
| want := &AsyncActionHandler[interface{}]{ | ||
| checkFn: checkFn, | ||
| sleepBeforeWait: 0 * time.Second, | ||
| throttle: 5 * time.Second, | ||
| timeout: 30 * time.Minute, | ||
| tempErrRetryLimit: 5, | ||
| checkFn: checkFn, | ||
| sleepBeforeWait: 0 * time.Second, | ||
| throttle: 5 * time.Second, | ||
| timeout: 30 * time.Minute, | ||
| tempErrRetryLimit: 5, | ||
| retryHttpErrorStatusCodes: RetryHttpErrorStatusCodes, | ||
| } | ||
|
|
||
| diff := cmp.Diff(got, want, cmpOpts...) | ||
|
|
@@ -160,7 +161,41 @@ func TestSetTempErrRetryLimit(t *testing.T) { | |
| got := New(checkFn) | ||
| got.SetTempErrRetryLimit(tt.tempErrRetryLimit) | ||
|
|
||
| diff := cmp.Diff(got, want, cmpOpts...) | ||
| diff := cmp.Diff(want, got, cmpOpts...) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nice, thx! |
||
| if diff != "" { | ||
| t.Errorf("Data does not match: %s", diff) | ||
| } | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestSetRetryHttpErrorStatusCodes(t *testing.T) { | ||
| checkFn := func() (waitFinished bool, res *interface{}, err error) { return true, nil, nil } | ||
|
|
||
| for _, tt := range []struct { | ||
| desc string | ||
| setRetryCodes []int | ||
| wantRetryCodes []int | ||
| }{ | ||
| { | ||
| "default", | ||
| []int{}, | ||
| []int{http.StatusBadGateway, http.StatusGatewayTimeout, http.StatusServiceUnavailable}, | ||
| }, | ||
| { | ||
| "base_3", | ||
| []int{http.StatusTooManyRequests, http.StatusInternalServerError}, | ||
| []int{http.StatusTooManyRequests, http.StatusInternalServerError}, | ||
| }, | ||
| } { | ||
| t.Run(tt.desc, func(t *testing.T) { | ||
| want := New(checkFn) | ||
| want.retryHttpErrorStatusCodes = tt.wantRetryCodes | ||
| got := New(checkFn) | ||
| if len(tt.setRetryCodes) != 0 { | ||
| got.SetRetryHttpErrorStatusCodes(tt.setRetryCodes) | ||
| } | ||
| diff := cmp.Diff(want, got, cmpOpts...) | ||
| if diff != "" { | ||
| t.Errorf("Data does not match: %s", diff) | ||
| } | ||
|
|
@@ -356,11 +391,12 @@ func TestWaitWithContext(t *testing.T) { | |
| return false, nil, fmt.Errorf("something bad happened when checking if the async action was finished") | ||
| } | ||
| handler := AsyncActionHandler[respType]{ | ||
| checkFn: checkFn, | ||
| sleepBeforeWait: tt.handlerSleepBeforeWait, | ||
| throttle: tt.handlerThrottle, | ||
| timeout: tt.handlerTimeout, | ||
| tempErrRetryLimit: tt.handlerTempErrRetryLimit, | ||
| checkFn: checkFn, | ||
| sleepBeforeWait: tt.handlerSleepBeforeWait, | ||
| throttle: tt.handlerThrottle, | ||
| timeout: tt.handlerTimeout, | ||
| tempErrRetryLimit: tt.handlerTempErrRetryLimit, | ||
| retryHttpErrorStatusCodes: RetryHttpErrorStatusCodes, | ||
| } | ||
| ctx, cancel := context.WithTimeout(context.Background(), tt.contextTimeout) | ||
| defer cancel() | ||
|
|
||
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.
I've always assumed this is public to allow setting it. With this change I'd be in favour of deprecating this and making it private after the deprecation period.
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.
On the other hand this allows to express: "whatever the default is + sth. else" like: