Skip to content
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

Merged
merged 4 commits into from
Jan 28, 2021

Conversation

DingoEatingFuzz
Copy link
Contributor

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.

@github-actions
Copy link

github-actions bot commented Jan 28, 2021

Ember Asset Size action

As of 3aa4cf3

Files that got Smaller 🎉:

File raw gzip
nomad-ui.js -6 B -357 B

Files that stayed the same size 🤷‍:

File raw gzip
vendor.js 0 B 0 B
nomad-ui.css 0 B 0 B
vendor.css 0 B 0 B

@github-actions
Copy link

github-actions bot commented Jan 28, 2021

Ember Test Audit comparison

master 3aa4cf3 change
passes 1544 1550 +6
failures 0 0 0
flaky 0 0 0
duration 8m 10s 750ms 8m 00s 486ms -10s 264ms

Copy link
Contributor

@backspace backspace left a 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'),
Copy link
Contributor

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?

Copy link
Contributor Author

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.
@DingoEatingFuzz
Copy link
Contributor Author

Okay! I reverted the life cycle change and added unit tests. Ready for re-review ✨

Copy link
Contributor

@backspace backspace left a 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 🥳

@DingoEatingFuzz
Copy link
Contributor Author

Oh! I like that idea. It also doubles as evidence in the UI code that we expect plain text responses from errors.

@vercel vercel bot temporarily deployed to Preview – nomad January 28, 2021 21:47 Inactive
@DingoEatingFuzz DingoEatingFuzz merged commit 4b7ee22 into master Jan 28, 2021
@DingoEatingFuzz DingoEatingFuzz deleted the b-ui/incorrect-error-message branch January 28, 2021 22:02
backspace added a commit that referenced this pull request Feb 10, 2021
This is a supplement to #9831 to incorporate the extracted
missing-permissions error handling from #9909.
backspace added a commit that referenced this pull request Feb 10, 2021
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
@github-actions
Copy link

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.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"allocation lifecycle permissions" when trying to restart with a stopped allocation
2 participants