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

sql: require super user return status #38454

Merged
merged 1 commit into from
Jun 30, 2019

Conversation

Gurio
Copy link
Contributor

@Gurio Gurio commented Jun 26, 2019

Resolves #32662 as discussed with @knz (hopefully correctly)
First commit is taken from #38124 which helped to find all the necessary places where code should be changed
I didn’t check if it builds with the last fix-commit, so please don’t review until TeamCity succeeds.

@Gurio Gurio requested review from a team June 26, 2019 19:43
@Gurio Gurio requested a review from a team as a code owner June 26, 2019 19:43
@Gurio Gurio requested a review from a team June 26, 2019 19:43
@CLAassistant
Copy link

CLAassistant commented Jun 26, 2019

CLA assistant check
All committers have signed the CLA.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@Gurio Gurio changed the title Require super user return status sql: require super user return status Jun 26, 2019
@Gurio Gurio changed the title sql: require super user return status WIP sql: require super user return status Jun 26, 2019
@Gurio Gurio changed the title WIP sql: require super user return status [WIP] sql: require super user return status Jun 26, 2019
Copy link
Contributor

@knz knz left a comment

Choose a reason for hiding this comment

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

Thanks the end result looks about right.

You're missing a piece though; as-is the code does not compile.

Then after that ensure the PR contains just your commit (instead of appending to the previous author's commit).
I could also recommend squashing the commits into one, there is no need for the intermediate situation which modifies many files to change the return values of RequireSuperUser.

Reviewed 3 of 20 files at r2, 1 of 1 files at r3.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained

@Gurio Gurio requested a review from a team as a code owner June 27, 2019 09:48
@Gurio Gurio force-pushed the require_super_user_return_status branch 2 times, most recently from f10cafe to c3e7e55 Compare June 27, 2019 09:57
@Gurio
Copy link
Contributor Author

Gurio commented Jun 27, 2019

@knz I've updated it and run test locally, should be fine now, lets see what CI says.
two open things:

  1. I've included @mgm702 's commit, to give kudos to his work. But as there is a problem with CLA, i will probably squash it into mine.
  2. I've updated RFCS/20171220_sql_role_based_access_control.md, according to the current AuthorizationAccessor interface. But I am really unsure if it is the right thing to do, because old RFCs probably should stay as they were. What do you think?

@Gurio
Copy link
Contributor Author

Gurio commented Jun 27, 2019

Ok build still fails, I’ll take a look

@Gurio Gurio closed this Jun 27, 2019
@Gurio Gurio reopened this Jun 27, 2019
@knz
Copy link
Contributor

knz commented Jun 27, 2019

RFC change is OK.

You can mention the name of the other contributor in the commit message, no need to include the commit itself.

@Gurio Gurio force-pushed the require_super_user_return_status branch from c3e7e55 to 172ccdd Compare June 27, 2019 12:17
@Gurio Gurio changed the title [WIP] sql: require super user return status sql: require super user return status Jun 27, 2019
@Gurio
Copy link
Contributor Author

Gurio commented Jun 27, 2019

@knz: I think PR is ready. Could you check/merge?

Copy link
Contributor

@knz knz left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 4 files at r4.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained

@knz
Copy link
Contributor

knz commented Jun 30, 2019

Thank you for your contribution!

bors r+

craig bot pushed a commit that referenced this pull request Jun 30, 2019
38454: sql: require super user return status r=knz a=Gurio

Resolves #32662 as discussed with @knz (hopefully correctly)
First commit is taken from #38124 which helped to find all the necessary places where code should be changed
I didn’t check if it builds with the last fix-commit, so please don’t review until TeamCity succeeds.

Co-authored-by: Arseni Lapunov <re.stage00101@gmail.com>
@craig
Copy link
Contributor

craig bot commented Jun 30, 2019

Build succeeded

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sql: RequireSuperUser() swallows errors
4 participants