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 modules: move params mapping from main to connect_to_db() function Backport/2.8/55799 #57473

Open
wants to merge 2 commits into
base: stable-2.8
from

Conversation

Projects
None yet
3 participants
@Andersson007
Copy link
Contributor

commented Jun 6, 2019

SUMMARY

PostgreSQL modules: move params mapping from main to connect_to_db() function
Backporting of #55799

ISSUE TYPE
  • Backport Pull Request

Andersson007 added some commits Apr 29, 2019

PostgreSQL modules: move params mapping from main to connect_to_db() …
…function (#55799)

* PostgreSQL modules: move params mapping from main to connect_to_db() function

* PostgreSQL modules: fix postgresql_db

* PostgreSQL modules: fixes

(cherry picked from commit 9b17346)

@Andersson007 Andersson007 changed the title Backport/2.8/55799 PostgreSQL modules: move params mapping from main to connect_to_db() function Backport/2.8/55799 Jun 6, 2019

@abadger

This comment has been minimized.

Copy link
Member

commented Jun 12, 2019

Several comment:

  • This looks like it changes connect_to_db()'s API in an incompatible manner (kw is no longer sent). That's something we try not to put into an already-released Ansible because there could be other, third-party modules which are relying on the API. We shouldn't break those in a minor release. (It would be nice to do a deprecation cycle... I think that we have documented a deprecation cycle as the official way to do module_utils incompatible changes but I'm not sure how closely we follow that.)

  • I'm also not sure if this is a better API. It replaces an explicit parameter with extracting the parameters from module.params in the module_utils function. that is a little magical. In other places we've been trying to make module_utils less reliant on being passed an AnsibleModule, keeping the unrelated functionality properly abstracted from each other. It also feels better to be explicit and have the parameter parsing be closer to the calling code.

    • Perhaps a compromise would be a new function whose sole purpose is to pull the postgres keyword args appropriate for the connection out of a dictionary. Then the caller could call this new function, passing in module.params. The filtered dictionary that is returned could then be passed to the connect_to_db() function.
      • I think that would both be a better, more explicit API, and remove the need to change the connect_to_db() function in a backwards incompatible way.

If you want an exception, you can ask at the public IRC meeting for committers to discuss and make a decision on whether this can be backported (Tuesdays and Thursdays): https://github.com/ansible/community/blob/master/meetings/README.md#tuesdays

@ansibot ansibot removed the needs_triage label Jun 12, 2019

@Andersson007

This comment has been minimized.

Copy link
Contributor Author

commented Jun 14, 2019

@abadger , thank you for the suggestion!
I'd like to clarify some points.

  • it is my function that was added recently, then moved to module_utils by #55514 and changed several times.

  • I agree with you about that passing connection params to connect_to_db probably is more explicit but on the other hand it is more complex then just to pass the module object.
    Moreover, connect_to_db uses module.fail_json and module.warn, not only module.params, so it looks to me like an excessive step (extract connection options anywhere else and pass to connect_to_db).
    New developers don't need to think about connection api at all, needs just to pass the module object similarly as in the other modules.
    If a potential new developer wants to know what's going on, he always may look at the connect_to_db, it does the same things as earlier in the modules.

  • I could add comments near

    db_connection = connect_to_db(module, autocommit=True)
    cursor = db_connection.cursor(cursor_factory=DictCursor)

that will explain something like (meaning) "the connection parameters are passed by a module object and handled into connect_to_db..." I think it's clear enough

@ansibot ansibot added the stale_ci label Jun 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.