-
Notifications
You must be signed in to change notification settings - Fork 6
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
clickhouse_user: set default role #70
clickhouse_user: set default role #70
Conversation
wait a minute, got some errors locally, fixing |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #70 +/- ##
==========================================
+ Coverage 85.42% 88.04% +2.61%
==========================================
Files 6 8 +2
Lines 597 711 +114
Branches 109 138 +29
==========================================
+ Hits 510 626 +116
+ Misses 45 43 -2
Partials 42 42 ☔ View full report in Codecov by Sentry. |
I'll finish it after PTO |
cc @aleksvagachev it's ready for review |
i'm now thinking that maybe that
On one hand that append_role it's how some of long-existing modules handle it - not saying it's good per se and probably it's not the best design but people are used to it. On the other hand, something like this feels more flexible. |
@Andersson007 I think it's interesting to add this. There is functionality for adding specific roles, but there is no functionality for removing specific roles) |
I think the latest suggested implementation is OK in this particular case that it'll basically add only |
@aleksvagachev changed, the tests were green, please take another look |
@Andersson007 Cool feature! Thanks for the contribution. |
@aleksvagachev the community is always welcome:) thanks for reviewing and merging! |
SUMMARY
See the changes