-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Allow wildcards in grants #1125
base: master
Are you sure you want to change the base?
Conversation
@@ -278,6 +275,41 @@ func (this *Inspector) validateGrants() error { | |||
return this.migrationContext.Log.Errorf("User has insufficient privileges for migration. Needed: SUPER|REPLICATION CLIENT, REPLICATION SLAVE and ALL on %s.*", sql.EscapeName(this.migrationContext.DatabaseName)) | |||
} | |||
|
|||
// grantContainsAll verifies the passed grant contain all the passed strings and | |||
// that the grant match the DB | |||
func (this *Inspector) grantContainsAll(grant string, substrings ...string) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Gonlo2 is it possible to add a unit test that confirms the logic in .grantContainsAll()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably I could move the major part of the code to https://github.com/github/gh-ost/blob/master/go/base/utils.go as a new function, maybe GrantMatch(grant string, databaseName string) bool
, and thus add some tests 🤔 . I don't know how you see it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Gonlo2 I think the purpose of go/base/utils.go
is for helpers that are called from many places in the code. I think it would make sense to move it there if it has many callers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@timvaillancourt I've added some tests, I don't know if you have any comments now with the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@timvaillancourt Any update about this?
Related issue: #713
Description
This PR adds support for using wildcards in grants by converting the mysql pattern into a regex and checking if it matches the named database.
script/cibuild
returns with no formatting errors, build errors or unit test errors.