status: move statusError to internal/status package #3432
Conversation
internal/status/status.go
Outdated
| return fmt.Sprintf("rpc error: code = %s desc = %s", codes.Code(p.GetCode()), p.GetMessage()) | ||
| } | ||
|
|
||
| func (se *StatusError) GRPCStatus() *spb.Status { |
There was a problem hiding this comment.
I'd imagine this change in public API may not be OK... Or it may since I'm moving this type to internal.
If I try to return a status.Status, I'll end up encountering an import cycle. @dfawley let me know what you think!
There was a problem hiding this comment.
ARGH! Import cycles. Would something like this work?
grpc/internal/status/statustype/statustype.go:
package statustype
type Status struct {
// move grpc/status/status.Status here
}
// Constructors/getters
grpc/status/status.go:
package status
type Status = statustype.Status
grpc/internal/status/status.go:
package status
func (se *StatusError) GRPCStatus() *statustype.Status {
...
}
There was a problem hiding this comment.
Alright, so I've tried this again - I run into an import cycle with statustype, because the Err() method returns a (*status.StatusError)(s.Proto()). If I make Status satisfy the error interface, the tests fail because of a type assertion to *status.StatusError.
Not sure of the right path forward!
There was a problem hiding this comment.
Hmmm, true. Status needs to reference StatusError and vice-versa. It's been awhile since I looked at this, but now I'm not sure why I was suggesting a third package at all. Can Status be defined in internal as well (in the same package as StatusError), with the same type alias from grpc/status for status.Status?
There was a problem hiding this comment.
Ok, I got that working! Tests pass (locally at least) so I'm muuuch more comfortable with this change.
There was a problem hiding this comment.
I agree, this looks good, thanks!
security/advancedtls/go.mod
Outdated
| @@ -2,4 +2,9 @@ module google.golang.org/grpc/security/advancedtls | |||
|
|
|||
| go 1.13 | |||
|
|
|||
| require google.golang.org/grpc v1.27.0 | |||
| require ( | |||
|
There's some go lint issues being reported by travis - do those need to be fixed before we can proceed? |
|
Yes; they should be resolved.
|
dfawley
changed the title
Fixes #3304