ansible-collections / community.mysql

MySQL Ansible Collection
https://galaxy.ansible.com/ui/repo/published/community/mysql/
Other
99 stars 88 forks source link

mysql_user: Adding statements output #75

Open steveteahan opened 3 years ago

steveteahan commented 3 years ago
SUMMARY

Adding a statements list to the output of mysql_user could make the module more transparent and easier to debug. Ideally, users would be able to run in check mode and see exactly which statements would be executed to get to the desired state.

ISSUE TYPE
COMPONENT NAME

mysql_user

ADDITIONAL INFORMATION

The mysql_user module currently returns changed, user, and msg. This proposal would add statements to include the SQL statements associated with the actual user or permission changes. An example of the proposed output may be seen below:

    "statements": [
        "CREATE USER 'db_user2'@'localhost' IDENTIFIED WITH mysql_native_password AS '[redacted]'",
        "GRANT EXECUTE ON FUNCTION `foo`.`function` TO 'db_user2'@'localhost'",
        "GRANT SELECT ON `foo`.* TO 'db_user2'@'localhost'",
        "GRANT EXECUTE ON PROCEDURE `bar`.`procedure` TO 'db_user2'@'localhost'",
        "GRANT USAGE ON *.* TO 'db_user2'@'localhost'"
    ],

An initial version (not ready for PR) can be seen here: https://github.com/steveteahan/community.mysql/commit/245f5f77a57b236fc926ad533a2a8fc4b7a26a0f

Providing the output of the SQL statements is something that the postgresql collection is doing today:

In that collection, they call it queries which I think is acceptable is well, it just seemed the statements was possibly more accurate.

Looking for some initial feedback about whether the maintainers are open to this change, or if it was already considered and avoided. Some other general thoughts about this:

Andersson007 commented 3 years ago

@steveteahan thanks for the feature idea! I'd suggest to name it queries because now it's implemented in mysql_variables module. As far as i remember, we can use cursor.mogrify() to get final statements without real execution.

I took a quick look at the pre-pr. Are tls related changes really necessary there? If not, better to put them in a separate PR and merge before of after the PR about logging. Also, would be cool to see the implementation as simple as possible (maybe your approach is, i don't know now, just a general recommendation).

steveteahan commented 3 years ago

Thank you, @Andersson007. To respond to your feedback:

I'll finish up my changes, get the docs and tests in order, and then submit a PR.

Andersson007 commented 3 years ago

@steveteahan cool, thanks:) Don't forget about a changelog fragment (minor_changes:) and integration tests