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

MODULES-8373 Fix mysql_grant resource to be idempodent on MySQL 8+ #1427

Merged

Conversation

aponert
Copy link
Contributor

@aponert aponert commented Aug 5, 2021

When the MySQL module is used against MySQL8, mysql_grant is no more idempodent.
This is because MySQL 8+ no more returns "ALL" for SHOW GRANTS, when all privileges
are granted. Instead, the separate privileges are shown.
Unfortunately, the tests were just adapted to not error in this situation.
This commit aims to fix the issue and also fixes the tests.

When the MySQL module is used against MySQL8, mysql_grant is no more idempodent.
This is because MySQL 8+ no more returns "ALL" for SHOW GRANTS, when all privileges
are granted. Instead, the separate privileges are shown.
Unfortunately, the tests were just adapted to not error in this situation.
This commit aims to fix the issue and also fixes the tests.
@aponert aponert requested a review from a team as a code owner August 5, 2021 15:23
@CLAassistant
Copy link

CLAassistant commented Aug 5, 2021

CLA assistant check
All committers have signed the CLA.

@david22swan
Copy link
Member

@theq86 I'm seeing some rubocop failures on your changes, but their easy to fix.
All you need to do is install the bundle environment and then run bundle exec rake rubocop -a.
This will autocorrect any of the failures that it can and tell you what ones you'll need to fix manually.

Once that's done we can see about getting this merged.

@aponert
Copy link
Contributor Author

aponert commented Aug 9, 2021

@david22swan Do you have a suggestion how to deal with "Style/GlobalVars: Do not introduce global variables." ?

The code that changes utf8 to utf8mb3 for MySQL 8 is needed in several places throughout the tests. To avoid code duplication I introduced the global charset variable in the spec_helper_acceptance.rb file.

I do not know ruby well enough to come up with a more elaborate way-

@david22swan
Copy link
Member

@theq86 Looking at this, firstly this code should not be in spec_helper_acceptance.rb, this file is managed entirely by the pdksync and should never be touched. Sorry for not catching it earlier.
Any changes such as this should instead go into the spec_helper_acceptance_local.rb, which is left untouched by the pdksync.

In regards to the code itself, wrapping it in a method call should work. Something like this:

def fetch_charset()
@charset ||= if os[:family] == 'ubuntu' && os[:release] =~ %r{^20\.04}
                        "utf8mb3"
                      else
                        "utf8"
                      end
end

This should set and return the value of @charset the first time the method is called and then just return it every other time after.

Copy link
Member

@david22swan david22swan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@theq86 This looks good to me so I'm gonna go ahead and merge it :)
Thank you for getting back on my comments so fast :)

@david22swan david22swan merged commit 7a026a9 into puppetlabs:main Aug 9, 2021
@aponert aponert deleted the bugfix/mysql8-idempodent-grants branch August 9, 2021 14:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants