-
Notifications
You must be signed in to change notification settings - Fork 186
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
Shift role initialization from accounts to settings #1696
Conversation
Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes. |
0a18cc2
to
9a507a4
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.
We should document the protocol change to vanilla grpc and add the circuit breaker to the changelog.
I am also not sure the settings service should have the default roles hardcoded. But changing that is a different topic.
Agree, the default data has to live somewhere, since we don't do storage, what would be the best candidate in your eyes? Since the settings service is responsible to roles, permissions and settings, it only makes sense that the required minimum data to startup and do minimal operations stays inside the service boundary. How it was before, the accounts NEEDED the roles and permissions to work, and that is assuming too much. Services should be self contained. |
Please retry analysis of this Pull-Request directly on SonarCloud. |
Description
Service initialization shouldn't block. When the accounts service starts, it notifies the settings service of a set of default roles and permissions, this piece is essential for the accounts to work but that by no means implies that its logic has to live within its business logic, since the storage layer resides in the settings service.
We want to invert the responsibility.
Changelog