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

mysql_user: Adding statements output #75

Open
steveteahan opened this issue Dec 20, 2020 · 3 comments
Open

mysql_user: Adding statements output #75

steveteahan opened this issue Dec 20, 2020 · 3 comments

Comments

@steveteahan
Copy link
Contributor

steveteahan commented Dec 20, 2020

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
  • Feature Idea
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: steveteahan@245f5f7

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:

  • Most of the functions in the module return early in check_mode, before the query is ever built, so additional refactoring would be required to make this work in check mode.
  • Check mode is arguably where this functionality would be most useful, so I'll take a look at the feasibility of moving the check mode conditionals around cursor.execute, but it's possible it wouldn't be that easy.
@Andersson007
Copy link
Collaborator

@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
Copy link
Contributor Author

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

  • queries works for me
  • Agreed on cursor.mogrify(), that's what I ended up using only wrapping it in a method to avoid mogrify() and append() calls everywhere
  • I'm happy to handle some of the refactoring in separate PRs to keep this change as straight-forward as possible

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

@Andersson007
Copy link
Collaborator

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

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 a pull request may close this issue.

2 participants