-
Notifications
You must be signed in to change notification settings - Fork 169
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
CIF - ServerError: storage.AccountsClient#ListAccountSAS: TooManyRequests #3298
Conversation
d433c9d
to
d209c8a
Compare
LGTM |
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.
LGTM
Please rebase pull request. |
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'm fine with longer errors, I prefer verbosity unless we can fit more information in less characters. Gave a suggestion for a shorter error message to satisfy Simon's request. LGTM!
d209c8a
to
9eb8db1
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.
@LiniSusan That's good. Thank you for this.
I left a suggestion to leverage early returns to make the code a bit more Go-ish, reducing the cyclomatic complexity.
🙂
90d36f4
to
dcf0c80
Compare
217f158
217f158
to
eafa458
Compare
LGTM |
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.
One last minor optional thing.
(Update) I wanted to press "Approve" and not "Request changes" as this is not a merge blocker, sorry. Changing it now .
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.
LGTM. One minor suggestions but definitely not a blocker 🙂
dc0efde
eafa458
to
dc0efde
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.
LGTM. Thanks!
Looks good 👍 |
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.
LGTM
dc0efde
to
bcac79e
Compare
reverting the runner changes changes for more robust code- checking for httpstatuscode simplifying the levels of nesting of the block of code
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.
LGTM
/azp run ci |
Azure Pipelines successfully started running 1 pipeline(s). |
Waiting for CI to go green, post that good to merge. |
Which issue this PR addresses:
https://issues.redhat.com/browse/ARO-3804
Fixes
What this PR does / why we need it:
To report the cluster installation failure error related to storage.AccountsClient#ListAccountSAS: Failure responding to request: StatusCode=429
Test plan for issue:
Is there any documentation that needs to be updated for this PR?