-
Notifications
You must be signed in to change notification settings - Fork 2k
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
[ui] Jobs list should handle 502s and 504s gracefully #23427
[ui] Jobs list should handle 502s and 504s gracefully #23427
Conversation
Ember Test Audit comparison
|
{{/if}} | ||
<A.Button @text="Manually fetch jobs" @color="secondary" {{on "click" this.restartJobList}} /> | ||
</Hds::Alert> | ||
{{/if}} | ||
|
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.
(Hide whitespace during review; I corrected an old indentation issue here)
this.watchJobs.cancelAll(); | ||
} | ||
|
||
@action restartJobList() { |
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.
Note: I let the user click "Try fetching again" before they've paused here even though the current retry on failure is 5sec with an eye to the future: we might extend that timeout beyond 5sec, or more likely add something like exponential backoff.
For now, though, it's an "I want to see the network request fire immediately to see if my new proxy config settings work" button.
* @param {Error} e | ||
*/ | ||
notifyFetchError(e) { | ||
const firstError = e.errors[0]; |
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 think I could .any()
this, but ¯_(ツ)_/¯
yield timeout(throttle); | ||
} else { | ||
if (Ember.testing) { | ||
break; | ||
} |
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.
A small change to my behaviour for testing purposes: I want to break out of a test on error now, too, which hadn't come up before. Without these lines, my test will be caught in a terrible loop.
normalizeQueryResponse( | ||
store, | ||
primaryModelClass, | ||
payload = [], |
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.
Only notable change here is a default of payload to []
so I don't have to do a bunch of conditional guards with elvis operators later
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 👍
cc4938d
to
35abc45
Compare
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
Previously, our jobs index page would handle handle all errors when fetching jobs by resetting state and trying again (with 5-second backoff).
This PR makes it so that proxy-timeout specific errors instead throw a notification, but keep the cached jobs shown on the page.
The user is then presented with two options to move forward:
Resolves #23228