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

deps: bump cockroachdb/errors and cockroachdb/redact #53764

Merged
merged 3 commits into from
Sep 2, 2020

Conversation

knz
Copy link
Contributor

@knz knz commented Sep 1, 2020

Fixes #53207
Fixes #53700
Needed for #53312

This picks up improved support for %T in format strings,
as well as better support for %w.

Release justification: low risk, high benefit changes to existing functionality

Release note: None

@knz knz requested review from tbg and irfansharif September 1, 2020 17:13
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Contributor

@irfansharif irfansharif left a comment

Choose a reason for hiding this comment

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

LGTM, though looks like we'll have to update TestRedactedDecodeFile.

@knz knz force-pushed the 20200901-bump-errors branch 2 times, most recently from ccdf1a6 to 8707ea6 Compare September 2, 2020 09:52
@knz knz requested review from a team and pbardea and removed request for a team September 2, 2020 10:30
@knz knz force-pushed the 20200901-bump-errors branch 2 times, most recently from d55f54f to 00a3932 Compare September 2, 2020 11:00
knz added 3 commits September 2, 2020 14:21
This picks up improved support for `%T` in format strings,
as well as better support for `%w`.

Release justification: low risk, high benefit changes to existing functionality

Release note: None
Prior to this patch, `colexecerror.InternalError()` would take any
type as input. This was encouraging callers to pass not only constant
strings and errors, it was also encouraging uses of `fmt.Sprintf`

However, given that these errors ultimately make their way to crash
reporting, we really want as much details as possible in crash
reports. Simple strings don't cut it and are considered fully unsafe
for reporting.

What we want instead if a discrete separation of safe and unsafe
details.  The machinery to do this is available in the `redact`
package, but for ease of use the `errors` library also does it.

So this patch changes `InternalError()` to require an `error`
argument, and invokes `errors.AssertionFailedf` or `log.PanicAsError`
in the call points.

This API can be further simplified in the future by making
`InternalError`
responsible for constructing the error object, for example

```go
InternalErrorf(format string, args ...interface{}) {
  panic(errors.AssertionFailedf(format, args...))
}
```

This improvement is left for a later change though.

Release justification: low risk, high benefit changes to existing functionality

Release note: None
Error objects in panics allow us to expose more safe details for
telemetry reporting.

Release justification: low risk, high benefit changes to existing functionality

Release note: None
@knz knz force-pushed the 20200901-bump-errors branch from 00a3932 to fea562b Compare September 2, 2020 12:22
@knz
Copy link
Contributor Author

knz commented Sep 2, 2020

bors r=irfansharif

@craig
Copy link
Contributor

craig bot commented Sep 2, 2020

Build succeeded:

@craig craig bot merged commit e04bc9a into cockroachdb:master Sep 2, 2020
@knz knz deleted the 20200901-bump-errors branch September 2, 2020 12:58
craig bot pushed a commit that referenced this pull request Sep 2, 2020
53312: server,pgwire: make more log message bits non-redactable r=spaskob,irfansharif a=knz

Commit prefix from  #53764
see individual commits for details

Release justification: low risk, high benefit changes to existing functionality

53650: sql: allow non-admins to perform some RESTOREs r=solongordon a=solongordon

Release justification: Low risk, high reward change to existing
functionality

Release note (sql change): Non-admin users are now permitted to execute
RESTORE statements as long as the restore does not depend on implicit
credentials and the user has the appropriate privileges to create all of
the resulting database objects. For database restores, this means the
user must have the CREATEDB role privilege. For table restores, the user
must have CREATE privileges on the parent database. Full cluster
restores still require admin privileges.

Co-authored-by: Raphael 'kena' Poss <[email protected]>
Co-authored-by: Solon Gordon <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants