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_user: allow to pass user name with dots #63565

Merged
merged 5 commits into from
Oct 18, 2019

Conversation

Andersson007
Copy link
Contributor

SUMMARY

postgresql_user: allow to pass user name with dots

fixes: #63204

it is obvious now - pg_quote_identifier doesn't work in this case

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

lib/ansible/module_utils/postgres.py
lib/ansible/modules/database/postgresql/postgresql_user.py

@Andersson007
Copy link
Contributor Author

Andersson007 commented Oct 16, 2019

@kostiantyn-nemchenko , @andytom , @kustodian , it does need your review

@ansibot
Copy link
Contributor

ansibot commented Oct 16, 2019

@ansibot ansibot added WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. affects_2.10 This issue/PR affects Ansible v2.10 bug This issue/PR relates to a bug. database Database category has_issue module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. postgresql PostgreSQL community support:community This issue/PR relates to code supported by the Ansible community. test This PR relates to tests. labels Oct 16, 2019
@Andersson007 Andersson007 changed the title [WIP] postgresql_user: allow to pass user name with dots postgresql_user: allow to pass user name with dots Oct 16, 2019
@Andersson007
Copy link
Contributor Author

ready_for_review

@ansibot ansibot added community_review In order to be merged, this PR must follow the community review workflow. and removed WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. labels Oct 16, 2019
@Spindel
Copy link

Spindel commented Oct 16, 2019

I think the code could simply remove those if statements on period, and just always quote identifiers properly?

@Andersson007
Copy link
Contributor Author

Andersson007 commented Oct 16, 2019

@Spindel which exact identifiers do you mean in case of role?
IMO identifiers were initially added to be able to handle names like “dbname.schemaname.tablename” -> “dbname”.”schemaname”.”tablename”
I’m not absolutely sure but it’s unclear for me which identifiers i can use with role. It is a global object.
Dot is a part of user name in this case, not an indicator of identifiers

@ansibot ansibot removed the needs_triage Needs a first human triage before being processed. label Oct 16, 2019
@Andersson007
Copy link
Contributor Author

Andersson007 commented Oct 16, 2019

@andytom , @Spindel , i've just changed all that stuff - set "%s" wherever was possible (in DDL queries). Looks better now, much less changes.. (it was the last, 6th PR probably, for today.. so was a bit exhausted doing that:)
@Spindel , if you meant this, sorry, i didn't understand
@andytom , thanks you for the explanation

@Andersson007
Copy link
Contributor Author

ready_for_review

@Spindel
Copy link

Spindel commented Oct 16, 2019 via email

Copy link
Contributor

@tcraxs tcraxs left a comment

Choose a reason for hiding this comment

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

LGTM

@ansibot ansibot added shipit This PR is ready to be merged by Core and removed community_review In order to be merged, this PR must follow the community review workflow. labels Oct 17, 2019
@andytom
Copy link
Contributor

andytom commented Oct 17, 2019

LGTM
shipit

Is this something that should be added to the other modules that deal with roles like postgresql_privs and postgresql_membership?

@Andersson007
Copy link
Contributor Author

@andytom postgresql_membership uses the same class, its (and others) CI were passed, i checked.
i'll check postgresql_privs later, after #63555 is merged, not to mix all in one pr

@Andersson007
Copy link
Contributor Author

@andytom @tcraxs thak you for approving

@Akasurde Akasurde merged commit 684e70c into ansible:devel Oct 18, 2019
@ansible ansible locked and limited conversation to collaborators Nov 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.10 This issue/PR affects Ansible v2.10 bug This issue/PR relates to a bug. database Database category has_issue module This issue/PR relates to a module. postgresql PostgreSQL community shipit This PR is ready to be merged by Core support:community This issue/PR relates to code supported by the Ansible community. test This PR relates to tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PostgreSQL user and db names should allow dots
6 participants