sql_mode can be set in session, therefore we should look for ANSI_QUOTES in session variable instead of global variable

[SoledaD208]

…TES in session variable instead of global variable

SUMMARY

ISSUE TYPE

• Bugfix Pull Request
• Docs Pull Request
• Feature Pull Request
• New Module Pull Request

COMPONENT NAME

ADDITIONAL INFORMATION

[laurent-indermuehle]

Hi @SoledaD208 and thank you for taking the time to open a PR.

Before we can merge, it misses:

• A description that explains why the change is made.
• A change log fragment with the same explanation.
• Integration tests.

About tests : I'm not sure what will happen when you change the scope of the sql_mode from global to session for the mysql_role and mysql_user modules. A rapid grep showed that the line you modified is the only time we use sql_mode in the entire collection.

Also, while writing tests, you could demonstrate how you intent to set a session with set sql_mode='ANSI_QUOTES'". As I don't think community.mysql allow for this. But tests are a great way to experiment that.

If I'm right, we could modify your PR to add such a feature. Feel free to ask me for help executing tests. I'm also available on matrix : #mysql:ansible.com

[SoledaD208]

Hi Laurent, Thanks for your answer. I'm a bit busy these days so can't reply to you promptly. Before we can merge, it misses: Sure, I will spend some time doing those. A rapid grep showed that the line you modified is the only time we use sql_mode in the entire collection. Agreed with you. That's also what I can see from the code. you could demonstrate how you intent to set a session with set sql_mode='ANSI_QUOTES'". As I don't think community.mysql allow for this Oh, I don't know about that, let me try if I can. In a 'real life' situation, I can set sql_mode by making use of parameter session_vars or setting sql_mode in a client config file, and point the module to that file. By the way, not related to sql_mode, I think mysql_user cannot parse the role grants or partial revokes (Please correct if I'm wrong). E.g: - partial revoke: mysql> SHOW GRANTS FOR u1; +------------------------------------------+ | Grants for u1@% | +------------------------------------------+ | GRANT SELECT, INSERT ON *.* TO `u1`@`%` | | REVOKE INSERT ON `world`.* FROM `u1`@`%` | +------------------------------------------+ - role grants: mysql> SHOW GRANTS FOR 'dev1'@'localhost'; +-------------------------------- -----------------+ | Grants for ***@***.*** | +-------------------------- -----------------------+ | GRANT USAGE ON *.* TO `dev1`@`localhost` | | GRANT `app_developer`@`%` TO `dev1`@`localhost` | +------------------------- ------------------------+ If those types of grants are not able to be parsed yet. Would they be supported in newer versions of the module? Thanks, Tuan ( aka soledad208 :) )

…

------------------------------ *From:* Laurent Indermühle ***@***.***> *Sent:* Wednesday, September 11, 2024 2:29:06 PM *To:* ansible-collections/community.mysql < ***@***.***> *Cc:* Soledad208 ***@***.***>; Mention < ***@***.***> *Subject:* Re: [ansible-collections/community.mysql] sql_mode can be set in session, therefore we should look for ANSI_QUOTES in session variable instead of global variable (PR #677) Hi @SoledaD208 <https://github.com/SoledaD208> and thank you for taking the time to open a PR. Before we can merge, it misses: - A description that explains why the change is made. - A change log fragment with the same explanation. - Integration tests. About tests : I'm not sure what will happen when you change the scope of the sql_mode from global to session for the mysql_role and mysql_user modules. A rapid grep showed that the line you modified is the only time we use sql_mode in the entire collection. Also, while writing tests, you could demonstrate how you intent to set a session with set sql_mode='ANSI_QUOTES'". As I don't think community.mysql allow for this. But tests are a great way to experiment that. If I'm right, we could modify your PR to add such a feature. Feel free to ask me for help executing tests. I'm also available on matrix : #mysql:ansible.com <https://matrix.to/#/%23mysql:ansible.com> — Reply to this email directly, view it on GitHub <#677 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACCTXOXYG6L7TUPBQ7ZUN43ZV7WMFAVCNFSM6AAAAABN4CGCSSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNBSHA3TANJUGI> . You are receiving this because you were mentioned.Message ID: ***@***.***>