-
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: Use server-sent error messages when applicable #9909
Conversation
Ember Asset Size actionAs of 3aa4cf3 Files that got Smaller 🎉:
Files that stayed the same size 🤷:
|
Ember Test Audit comparison
|
27b2a80
to
44e164c
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.
I appreciate this extraction. I wonder whether it might be improved by having a test that exercises the error.errors
branch when it’s not a 403?
@@ -85,7 +86,7 @@ export default class IndexController extends Controller.extend(Sortable) { | |||
} catch (err) { | |||
this.set('error', { | |||
title: 'Could Not Restart Allocation', | |||
description: 'Your ACL token does not grant allocation lifecycle permissions.', | |||
description: messageForError(err, 'manage allocation life cycle'), |
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.
The documentation seems to consistently use lifecycle
vs life cycle
, should this too?
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.
Good call.
Automatically detect ACL errors Provide a generic error message as a fallback
When the error is actually a 403, an ACL error is appropriate, but when it isn't, fallback on what the API returns.
44e164c
to
5dfa556
Compare
Okay! I reverted the |
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 should have been more specific in that I was imagining an acceptance test so it’s more end-to-end-y but really I think this is fine, thanks 🥳
Oh! I like that idea. It also doubles as evidence in the UI code that we expect plain text responses from errors. |
This is a supplement to #9831 to incorporate the extracted missing-permissions error handling from #9909. It fixes this failure on the main branch! 😳 https://app.circleci.com/pipelines/github/hashicorp/nomad/14728/workflows/4c147dca-fd1e-4de7-86aa-90ded7aabad2/jobs/137137
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. |
Fixes #7875
In most places in the UI, the error message shown after failed actions is a mostly generic "Your ACL token said no" message. Sometimes this occludes genuine and helpful 500 error messages being returned from the API.
This makes it so anywhere we show an error message we make sure to only show an ACL error message if it is a 403 error, otherwise just print what the API gave us.