sql: dropping user with default privilege's error message is more clear #77016
Conversation
|
This change is |
pkg/sql/drop_role.go
Outdated
| ) | ||
| } else { | ||
| err = errors.Newf( | ||
| "privileges for default privileges on new %s belonging to role %s", | ||
| objectType, role.Role, | ||
| wrapErrorMsgWithSchemaName("privileges for default privileges on new %s belonging to role %s in database %s"), |
There was a problem hiding this comment.
any reason not to use errors.WithDetail here? should we add a errors.WithHint to suggest what syntax can be used to remove the default privileges?
There was a problem hiding this comment.
We can add a hint for each but I'm slightly afraid it might make it too verbose, but I think we'd rather err that way than making it more obfuscated so I'm for it.
There was a problem hiding this comment.
Yeah alright but I'd say the table info should be a detail
RichardJCai
force-pushed
the
make_default_privileges_error_msg_more_clear
branch
from
dbade07 to
586f44c
Compare
There was a problem hiding this comment.
lgtm! is the 21.1 backport actually needed? there's no default privs there right?
Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @otan, @RichardJCai, and @ZhouXing19)
Release note (sql change): When dropping a user that has default privileges, the error message now includes which database and schema the default privileges are defined in. Additionally a hint is given to show exactly how to remove the default privileges. Example: pq: role testuser4 cannot be dropped because some objects depend on it owner of default privileges on new sequences belonging to role testuser4 in database testdb2 in schema s privileges for default privileges on new sequences belonging to role testuser3 in database testdb2 in schema s privileges for default privileges on new sequences for all roles in database testdb2 in schema public privileges for default privileges on new sequences for all roles in database testdb2 in schema s HINT: USE testdb2; ALTER DEFAULT PRIVILEGES FOR ROLE testuser4 IN SCHEMA S REVOKE ALL ON SEQUENCES FROM testuser3; USE testdb2; ALTER DEFAULT PRIVILEGES FOR ROLE testuser3 IN SCHEMA S REVOKE ALL ON SEQUENCES FROM testuser4; USE testdb2; ALTER DEFAULT PRIVILEGES FOR ALL ROLES IN SCHEMA PUBLIC REVOKE ALL ON SEQUENCES FROM testuser4; USE testdb2; ALTER DEFAULT PRIVILEGES FOR ALL ROLES IN SCHEMA S REVOKE ALL ON SEQUENCES FROM testuser4;
RichardJCai
force-pushed
the
make_default_privileges_error_msg_more_clear
branch
from
586f44c to
93337c8
Compare
Yeah don't need 21.1 |
|
TFTR! bors r=rafiss |
|
Build succeeded: |
|
Encountered an error creating backports. Some common things that can go wrong:
You might need to create your backport manually using the backport tool. error creating merge commit from 93337c8 to blathers/backport-release-21.2-77016: POST https://api.github.com/repos/cockroachlabs/cockroach/merges: 403 Resource not accessible by integration [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch 21.2.x failed. See errors above. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan. |
Release note (sql change): When dropping a user that has default privileges,
the error message now includes which database and schema the default privileges
are defined in.
Error message now includes the database name and schema name for which the default privileges are defined.
The UX is still not ideal here but this is a good first step.
Example: