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

mgr/dashboard: Rename and refactor ApiInterceptorService class #21386

Merged
merged 3 commits into from Apr 20, 2018

Conversation

votdev
Copy link
Member

@votdev votdev commented Apr 12, 2018

Display notifications for more errors than 401 and 500.

Signed-off-by: Volker Theile vtheile@suse.com

@votdev votdev force-pushed the api_interceptor branch 2 times, most recently from 4551a2c to e1d4cde Compare April 12, 2018 16:50
@votdev votdev changed the title mgr/dashboard: Introduce API HTTP interceptor mgr/dashboard: Rename and refactor ApiInterceptorService class Apr 12, 2018
@LenzGr LenzGr added this to the mimic milestone Apr 16, 2018
break;
}
this.notificationService.show(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We cannot display a generic error when we receive a 409 error code.

409 is used on RBD management to display validation errors returned by ceph.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same problem with RGW; it returns 404 for some errors which is totally wrong in case of the UI because it will redirect the UI to the 404 page. I fixed that by catching all exceptions coming from the RGW proxy and return them as 500.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we extend this to use something like preventDefault to prevent the routing in specific cases?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Devp00l Could you please explain it more detailed because this PR does not change general logic of the existing code. maybe your suggestion should be done with a separate PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@votdev we cannot implement an equivalent solution for RBDs because returning 409 in RBD is intentional.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@ricardoasmarques ricardoasmarques left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

409 should be handled manually

break;
}
this.notificationService.show(
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we extend this to use something like preventDefault to prevent the routing in specific cases?

Copy link
Contributor

@ricardoasmarques ricardoasmarques left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

…ifications for more errors than 401 and 500.

Signed-off-by: Volker Theile <vtheile@suse.com>
Signed-off-by: Volker Theile <vtheile@suse.com>
Signed-off-by: Volker Theile <vtheile@suse.com>
@votdev
Copy link
Member Author

votdev commented Apr 19, 2018

@Devp00l I've added the ability to cancel a notification.

Copy link

@Devp00l Devp00l left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@LenzGr LenzGr merged commit 7075470 into ceph:master Apr 20, 2018
@votdev votdev deleted the api_interceptor branch April 20, 2018 10:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants