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

clickhouse_user: set default role #70

Merged
merged 18 commits into from
Aug 22, 2024

Conversation

Andersson007
Copy link
Contributor

@Andersson007 Andersson007 commented Jul 16, 2024

SUMMARY

See the changes

@Andersson007 Andersson007 marked this pull request as draft July 16, 2024 08:44
@Andersson007 Andersson007 marked this pull request as ready for review July 19, 2024 07:50
@Andersson007
Copy link
Contributor Author

cc @aleksvagachev

@Andersson007 Andersson007 marked this pull request as draft July 19, 2024 07:53
@Andersson007
Copy link
Contributor Author

wait a minute, got some errors locally, fixing

Copy link

codecov bot commented Jul 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.04%. Comparing base (8435902) to head (5887e2f).

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.
📢 Have feedback on the report? Share it here.

@Andersson007
Copy link
Contributor Author

I'll finish it after PTO

@Andersson007 Andersson007 marked this pull request as ready for review August 19, 2024 14:38
@Andersson007
Copy link
Contributor Author

cc @aleksvagachev it's ready for review

@Andersson007
Copy link
Contributor Author

i'm now thinking that maybe that append_role thing that used say in ansible.builtin.user module can be made more flexible? how about something like:

role_mode: append | remove | listed_only (default)
default_role_mode:  append | remove | listed_only (default)

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.
What do you think @aleksvagachev ? Maybe you have a third option on your mind?

@aleksvagachev
Copy link
Collaborator

aleksvagachev commented Aug 21, 2024

@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)
It is probably necessary to form the idea that the module can perform only basic actions. And for more specific settings, it is better to use the clickhouse_client module. Otherwise, sooner or later it will become difficult to maintain the code, and users will get confused.

@Andersson007
Copy link
Contributor Author

And for more specific settings, it is better to use the clickhouse_client module. Otherwise, sooner or later it will become difficult to maintain the code, and users will get confused.

I think the latest suggested implementation is OK in this particular case that it'll basically add only remove, so will not complicate the code much. Let me try

@Andersson007
Copy link
Contributor Author

@aleksvagachev changed, the tests were green, please take another look

@aleksvagachev
Copy link
Collaborator

@Andersson007 Cool feature! Thanks for the contribution.

@aleksvagachev aleksvagachev merged commit 1fad110 into ansible-collections:main Aug 22, 2024
21 checks passed
@Andersson007
Copy link
Contributor Author

@aleksvagachev the community is always welcome:) thanks for reviewing and merging!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants