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

Idempotence all grant #55140

Open
wants to merge 6 commits into
base: devel
from

Conversation

Projects
None yet
6 participants
@azeliashchonak
Copy link

azeliashchonak commented Apr 11, 2019

SUMMARY

In MySQL 8.0 SHOW GRANTS no longer displays ALL PRIVILEGES in its global-privileges output. Instead, SHOW GRANTS explicitly lists each granted global privilege. The issue is that the mysql_user module is using a simple comparison between new and current privileges and if we define ".:ALL,GRANT" it will return changed, because "ALL" != list_of_privileges, so idempotence check will be failed.

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME

mysql_user

ADDITIONAL INFORMATION

Instead of #52469

azeliashchonak added some commits Apr 11, 2019

@ansibot

This comment has been minimized.

@gundalow
Copy link
Contributor

gundalow left a comment

Is it possible your branch wasn't up to date before creating this PR, I see lots of unrelated changed in here

See https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html for guidance on how to rebase your branch

Show resolved Hide resolved lib/ansible/modules/database/mysql/mysql_user.py Outdated
Show resolved Hide resolved lib/ansible/modules/database/mysql/mysql_user.py Outdated

@ansibot ansibot removed the needs_triage label Apr 11, 2019

@azeliashchonak azeliashchonak changed the title Idempotence all grant [WIP]Idempotence all grant Apr 11, 2019

@ansibot ansibot added the WIP label Apr 11, 2019

azeliashchonak added some commits Apr 11, 2019

@azeliashchonak azeliashchonak changed the title [WIP]Idempotence all grant Idempotence all grant Apr 11, 2019

@ansibot ansibot removed the WIP label Apr 11, 2019

@azeliashchonak

This comment has been minimized.

Copy link
Author

azeliashchonak commented Apr 15, 2019

/bot_status

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Apr 15, 2019

Components

lib/ansible/modules/database/mysql/mysql_user.py
support: community
maintainers: Jmainguy Xyon bmalynovytch bmildren michaelcoburn oneiroi tolland

Metadata

waiting_on: azeliashchonak
changes_requested_by: gundalow
needs_info: False
needs_revision: True
needs_rebase: False
merge_commits: []
too many files or commits: False
mergeable_state: clean
shippable_status: success
maintainer_shipits (module maintainers): False
community_shipits (namespace maintainers): False
ansible_shipits (core team members): False
shipit_actors (maintainer or core team member): None
shipit_actors_other:
automerge: automerge shipit test failed

click here for bot help

@azeliashchonak

This comment has been minimized.

Copy link
Author

azeliashchonak commented Apr 15, 2019

/ready_for_review

@tgadiev

This comment has been minimized.

Copy link

tgadiev commented Apr 15, 2019

@bmalynovytch can you take a look please?

- name: Removes all anonymous user accounts
mysql_user:
name: ''
host_all: yes
state: absent
- name: Create database user with name 'bob' and password '12345' with all database privileges

This comment has been minimized.

Copy link
@bmalynovytch

bmalynovytch Apr 15, 2019

Contributor

@azeliashchonak
Many whitespace changes there, probably introduced by automatic formatting.
Could you revert those changes, unless they are necessary / related to the object of the current PR ?

This comment has been minimized.

Copy link
@azeliashchonak

azeliashchonak Apr 19, 2019

Author

Why should we leave empty rows in comments?

This comment has been minimized.

Copy link
@bmalynovytch

bmalynovytch Apr 19, 2019

Contributor

Oh ! We both misread / misunderstood theses changes: they are not whitespace changes, neither empty rows.
Removing these lines completely changes the structure, removing one level of list (list of lists becomes simple list).
I don't know if this extra level is necessary or not, we need to check how documentation should be provided.

@felixfontein @gundalow Do you have any input about this ?

This comment has been minimized.

Copy link
@felixfontein

felixfontein Apr 19, 2019

Contributor

@azeliashchonak this is not a comment, but extracted and put into the documentation of the module. See here: https://docs.ansible.com/ansible/devel/modules/mysql_user_module.html#examples

In general, examples are better readable if different examples are separated with empty lines. So I would definitely keep them.

Show resolved Hide resolved lib/ansible/modules/database/mysql/mysql_user.py Outdated
Show resolved Hide resolved lib/ansible/modules/database/mysql/mysql_user.py Outdated
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.