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

Add missing MariaDB Grants to mysql module #61410

Merged
merged 2 commits into from
Oct 17, 2022

Conversation

cebe
Copy link
Contributor

@cebe cebe commented Jan 1, 2022

What does this PR do?

MariaDB has added some grants in 10.4.x and 10.5.x that are not present here, which results in an error when creating.

Also improved exception handling in grant_add which did not log the original error message and replaced it with a generic error.

What issues does this PR fix or reference?

Fixes: #61409

Previous Behavior

Error when trying to add grants that salt is not aware of.
The error message is very unspecific: "Error during grant generation"

New Behavior

No error for new MariaDB grants, better error message in case of unsupported grants.

Merge requirements satisfied?

Commits signed with GPG?

Yes

@cebe cebe requested a review from a team as a code owner January 1, 2022 13:31
@cebe cebe requested review from krionbsd and removed request for a team January 1, 2022 13:31
@cebe cebe marked this pull request as draft January 1, 2022 13:32
@cebe cebe marked this pull request as ready for review January 6, 2022 00:37
@ITJamie
Copy link
Contributor

ITJamie commented Aug 25, 2022

another issue for this: #58297

@Ch3LL Ch3LL requested review from garethgreenaway and removed request for krionbsd August 29, 2022 15:52
@garethgreenaway
Copy link
Contributor

@cebe Looks good. Would be great to add some tests, please let me know if you need help getting those in.

@garethgreenaway garethgreenaway added this to the Sulphur v3006.0 milestone Sep 2, 2022
@garethgreenaway garethgreenaway added Sulfur v3006.0 release code name and version Needs-Testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases labels Sep 2, 2022
@Ch3LL
Copy link
Contributor

Ch3LL commented Sep 21, 2022

bump @cebe can you respond to @garethgreenaway comment above?

@garethgreenaway
Copy link
Contributor

#59280 has been merged in which addresses similar issues to this PR.

@Ch3LL
Copy link
Contributor

Ch3LL commented Sep 29, 2022

@cebe can we close this PR In favor of #59280?

@garethgreenaway
Copy link
Contributor

Closing this one out in favor of #59280

@cebe
Copy link
Contributor Author

cebe commented Oct 7, 2022

@cebe can we close this PR In favor of #59280?

#59280 does not contain all of what is in here, I'll update my branch with the changes from #59280 to show the diff. @garethgreenaway Please reopen.

@Ch3LL
Copy link
Contributor

Ch3LL commented Oct 10, 2022

@cebe can you resovle the merge conflict?

@garethgreenaway will this require tests or is there already test coverage for a change like this?

@cebe
Copy link
Contributor Author

cebe commented Oct 14, 2022

@Ch3LL I just rebased my commit on master, here are the changes that are left:

as far as I see, no extra tests are needed for this.

MariaDB has added some grants in 10.4.x and 10.5.x that are not present here, which results in an error when creating.

This is an addition to saltstack#59280.

Also improved exception handling in `grant_add` which did not log the original error message and replaced it with a generic error.
@garethgreenaway
Copy link
Contributor

garethgreenaway commented Oct 17, 2022

@Ch3LL I think we're good with existing test coverage.

@cebe The changelog is correct, the other PR didn't have one but we added one after the fact. Tests are failing for unrelated reasons, once they're passing we'll get this merged.

@garethgreenaway garethgreenaway merged commit f2d2532 into saltstack:master Oct 17, 2022
@cebe
Copy link
Contributor Author

cebe commented Oct 17, 2022

Nice, thanks for merging!

@cebe cebe deleted the patch-1 branch October 17, 2022 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs-Testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases pending-close Sulfur v3006.0 release code name and version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[TECH DEBT] mysql modules is not up to date with MariaDB Grants
4 participants