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

executor: add DROP ROLE support #9616

Merged
merged 7 commits into from
Mar 13, 2019
Merged

executor: add DROP ROLE support #9616

merged 7 commits into from
Mar 13, 2019

Conversation

imtbkcat
Copy link

@imtbkcat imtbkcat commented Mar 8, 2019

What problem does this PR solve?

add DROP ROLE function for parser and executor.

What is changed and how it works?

Role is same as user, is store in mysql.user table. We do same action as DROP USER when we need to drop a role. Also, as we has added some system table for RBAC, like mysql.role_edges and
mysql.default_role. We have to delete information from these tables when execute DROP ROLE

Check List

Tests

  • Unit test

Code changes

  • Has exported variable/fields change

Side effects

  • Increased code complexity

@imtbkcat
Copy link
Author

imtbkcat commented Mar 8, 2019

parser pr: pingcap/parser#237

@zhouqiang-cl
Copy link
Contributor

/run-all-tests

Copy link
Contributor

@tiancaiamao tiancaiamao left a comment

Choose a reason for hiding this comment

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

Could drop role statement drop an user, or drop user statement drop an role in MySQL? @imtbkcat

@imtbkcat
Copy link
Author

@tiancaiamao yes, drop role can drop user and drop user can also drop role.

@tiancaiamao
Copy link
Contributor

LGTM

@tiancaiamao tiancaiamao added the status/LGT1 Indicates that a PR has LGTM 1. label Mar 12, 2019
@jackysp
Copy link
Member

jackysp commented Mar 12, 2019

pingcap/parser#237 is merged @imtbkcat

@imtbkcat
Copy link
Author

/run-all-tests

@codecov
Copy link

codecov bot commented Mar 12, 2019

Codecov Report

Merging #9616 into master will decrease coverage by 0.0368%.
The diff coverage is 21.875%.

@@               Coverage Diff                @@
##             master      #9616        +/-   ##
================================================
- Coverage   67.3912%   67.3544%   -0.0369%     
================================================
  Files           377        377                
  Lines         79353      79383        +30     
================================================
- Hits          53477      53468         -9     
- Misses        21104      21134        +30     
- Partials       4772       4781         +9

@imtbkcat
Copy link
Author

PTAL @jackysp

Copy link
Member

@jackysp jackysp left a comment

Choose a reason for hiding this comment

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

LGTM

@imtbkcat imtbkcat merged commit d4bad4e into pingcap:master Mar 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/execution SIG execution status/LGT1 Indicates that a PR has LGTM 1.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants