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

release-20.1: sql: Change output of CREATE/ALTER/DROP ROLE to not include rows affected. Add hint for GRANT ROLE. #46819

Merged
merged 1 commit into from
Apr 7, 2020

Conversation

RichardJCai
Copy link
Contributor

@RichardJCai RichardJCai commented Mar 31, 2020

Backport 1/1 commits from #46795.
/cc @cockroachdb/release

@RichardJCai RichardJCai requested review from knz and a team March 31, 2020 22:58
@RichardJCai RichardJCai requested a review from a team as a code owner March 31, 2020 22:58
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@knz
Copy link
Contributor

knz commented Mar 31, 2020

Can you fix the subject line of the commit message. right now it spans two lines

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.

Please also add the examples from the PR description into the commit message. That's where they belong.

I also vaguely recall I have reviewed this PR already somewhere - did you create a new one?

Reviewed 7 of 7 files at r1.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained

@knz
Copy link
Contributor

knz commented Mar 31, 2020

did you use the backport tool?

@RichardJCai RichardJCai requested a review from knz April 2, 2020 23:02
@RichardJCai
Copy link
Contributor Author

Please also add the examples from the PR description into the commit message. That's where they belong.

I also vaguely recall I have reviewed this PR already somewhere - did you create a new one?

Reviewed 7 of 7 files at r1.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained

Sorry, I didn't know about the backport tool - updated to use it.

@knz
Copy link
Contributor

knz commented Apr 4, 2020

do you still need a review on this one?

@RichardJCai
Copy link
Contributor Author

Yeah, or rather need approval to backport it.

@knz
Copy link
Contributor

knz commented Apr 4, 2020

ok then can you update the PR description and rebase, then i'll review

… Add hint for GRANT ROLE.

Fixes cockroachdb#46728, cockroachdb#46698

Release note (sql change): Remove rows affected from CREATE/ALTER/DROP ROLE
command results.
The number doesn't make sense to include since rows affected includes
system.role_options rows, this number can be misleading (ie CREATE ROLE returning 3).
This also matches PG as no number is returned for these commands.

Release note (sql change): Add hint to use ALTER ROLE when trying to
"GRANT" a role option directly to a user (using the GRANT ROLE syntax).

Release justification: Low risk change, command result output changed for
CREATE/ALTER/DROP ROLE. Hint added for GRANT ROLE.
@RichardJCai RichardJCai reopened this Apr 6, 2020
@RichardJCai RichardJCai changed the title sql: Change output of CREATE/ALTER/DROP ROLE to not include rows affected. Add hint for GRANT ROLE. release-20.1: sql: Change output of CREATE/ALTER/DROP ROLE to not include rows affected. Add hint for GRANT ROLE. Apr 6, 2020
@RichardJCai
Copy link
Contributor Author

ok then can you update the PR description and rebase, then i'll review

Rebased and updated

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.

:lgtm:

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

@RichardJCai
Copy link
Contributor Author

TFTR!
bors r=knz

@knz
Copy link
Contributor

knz commented Apr 7, 2020

i think you need to retry this.

@RichardJCai
Copy link
Contributor Author

bors r=knz

@craig
Copy link
Contributor

craig bot commented Apr 7, 2020

Build failed

@RichardJCai
Copy link
Contributor Author

Retrying again - failure doesn't look related to change.

bors r=knz

craig bot pushed a commit that referenced this pull request Apr 7, 2020
46819: release-20.1: sql: Change output of CREATE/ALTER/DROP ROLE to not include rows affected. Add hint for GRANT ROLE.  r=knz a=RichardJCai

Backport 1/1 commits from #46795.
/cc @cockroachdb/release

Co-authored-by: Richard Cai <[email protected]>
@RichardJCai
Copy link
Contributor Author

bors r=knz

@craig
Copy link
Contributor

craig bot commented Apr 7, 2020

Build succeeded

@craig craig bot merged commit b47493f into cockroachdb:release-20.1 Apr 7, 2020
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.

3 participants