-
Notifications
You must be signed in to change notification settings - Fork 690
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
Upgrade Envoy go-control-plane and fix related changes to the spec #2432
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2432 +/- ##
=======================================
Coverage 76.85% 76.86%
=======================================
Files 67 67
Lines 5484 5486 +2
=======================================
+ Hits 4215 4217 +2
Misses 1173 1173
Partials 96 96
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@@ -17,8 +17,9 @@ import ( | |||
"sort" | |||
"sync" | |||
|
|||
rsrc "github.com/envoyproxy/go-control-plane/pkg/resource/v2" | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have a standard style for grouping imports?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if we have a standard, I copied this from the go-control-plane code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I renamed it to resource
. I didn't think the other alias made sense.
@@ -55,6 +55,9 @@ func (a Assert) Equal(want, got interface{}) { | |||
} | |||
|
|||
func unmarshalAny(a *any.Any) proto.Message { | |||
if a == nil { | |||
return nil | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happened here? Does this get a nil that it didn't get before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one was weird, after upgrading the deps, some tests started to fail. The error was the comparison was nil when checking.
Maybe if you could put a set of eyes on it and see if something else jumps out at you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that we just got lucky before. Any comparison involving an any.Any
could have triggered this, since it makes ptypes.Empty
return an error. Looks like RetryPolicyTypedConfig *any.Any
was added to a few types, which was probably one trigger.
Signed-off-by: Steve Sloka <[email protected]>
59287b7
to
bd32bf6
Compare
Fixes #2429
Signed-off-by: Steve Sloka [email protected]