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

postgresql_membership: remove unnecessary aliases #296

Open
Andersson007 opened this issue Jun 20, 2022 · 6 comments
Open

postgresql_membership: remove unnecessary aliases #296

Andersson007 opened this issue Jun 20, 2022 · 6 comments

Comments

@Andersson007
Copy link
Collaborator

Andersson007 commented Jun 20, 2022

SUMMARY

I noticed that the postgresql_membership module has the following sets of aliases for two parameters.

 23   groups:
 24     description:
 25     - The list of groups (roles) that need to be granted to or revoked from I(target_roles).
 29     aliases:
 30     - group
 31     - source_role
 32     - source_roles

 33   target_roles:
 34     description:
 35     - The list of target roles (groups will be granted to them).
 39     aliases:
 40     - target_role
 41     - users
 42     - user

It is a bad practice as aliases can be introduced for compatibility in case of typos/ unsuitable original argument names, etc.
I suggest choosing one name for each of the options and deprecate (and remove in 3.0.0) the other ones.

I suggest using:

  • source_roles
  • target_roles

What do you folks think?

@hunleyd
Copy link
Collaborator

hunleyd commented Jun 20, 2022

So groups is the list of roles that we are GRANTing or REVOKEing and target_roles is the recipient of said GRANT/REVOKE. I get where you got 'source' from but I don't think it is "obvious" to end users. So maybe we should groups -> roles so it looks like:

postgresql_membership:
  roles:
    - hr
    - payroll
   target_roles:
    - bob
    - carol
   state: present

I think that's pretty clear what is happening.

I could even see target_roles -> targets so it looks like:

postgresql_membership:
  roles:
    - hr
    - payroll
   targets:
    - bob
    - carol
   state: present

(I prefer the first example, tbh)

Thoughts @jchancojr @Andersson007 @keithf4 @pgguru ?

@Andersson007
Copy link
Collaborator Author

the first example (roles-target_roles) LGTM

@jchancojr
Copy link
Collaborator

Agree re: first example. Just reads a bit easier imo

@pgguru
Copy link

pgguru commented Jun 22, 2022

What about members instead of target_roles? Since the module is called postgresql_membership you end up granting the roles to the members.

@hunleyd
Copy link
Collaborator

hunleyd commented Jun 22, 2022

i actually really like @pgguru 's suggestion

@Andersson007
Copy link
Collaborator Author

i actually really like @pgguru 's suggestion

I'll be happy with both:) Thanks for a good option!

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

No branches or pull requests

4 participants