-
Notifications
You must be signed in to change notification settings - Fork 89
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
SALTO-5915 Google ws - fix - roleAssignment deployment #5881
SALTO-5915 Google ws - fix - roleAssignment deployment #5881
Conversation
913cdc6
to
f113151
Compare
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.
looks great! 👯
for next prs, I think it's better to split adapter changes from infra changes, and link each one to it's own ticket. It makes it easier to understand later why we done each change
Also, please update PR title to include info about the infra change 🙏
const response = await singleClientCall(args) | ||
return polling.checkStatus(response) ? response : undefined | ||
} catch (e) { | ||
if (polling.retryOnStatus?.includes(e.response?.status)) { |
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.
let's verify as well that ?
e instanceof clientUtils.HTTPError
@@ -38,6 +38,7 @@ export type PollingArgs = { | |||
interval: number | |||
retries: number | |||
checkStatus: (response: Response<ResponseValue | ResponseValue[]>) => boolean | |||
retryOnStatus?: number[] |
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.
nit - maybe worth adding a comment about why this is needed and how it's different from checkStatus
?
SALTO-5915 Google ws - fix - roleAssignment deployment
Additional context for reviewer
Release Notes:
none
User Notifications:
none