contrib/google.golang.org/api: update google.golang.org/api version #1971
Conversation
…nd endpoints_gen.go generated code
|
Thank you, @rarguelloF :) |
There was a problem hiding this comment.
So halfway through reviewing this code and leaving comments I realized most of the code from the new main.go file is copied from make_endpoints.go 😛 . So while we can make the changes I commented, it feels like a good idea to not modify too heavily.
One question: I think this might be considered a breaking change for customer reliant on this API right? As endpoints may now have different names? It doesn't seem like it will break compilation though? I'm thinking that the best approach for us here will be to modify the package docs for this package to make it very clear to customers that we will periodically regenerate the "known endpoints" from the google "source of truth" and that they do not follow sem ver for those changes and therefore changes may occur in a way that causes spans/metrics to have their names changed (if I'm properly understanding the difference, please feel free to word this in whatever makes the most sense to you since you've spent more time looking at this area than I have at this point)
…dpointMetadataEnabled option
|
@ajgajg1134 Thanks for your review! |
| @@ -116,6 +118,31 @@ func TestURLShortener(t *testing.T) { | |||
| assert.Equal(t, ext.SpanKindClient, s0.Tag(ext.SpanKind)) | |||
| } | |||
|
|
|||
| func TestWithEndpointMetadataEnabled(t *testing.T) { | |||
There was a problem hiding this comment.
| func TestWithEndpointMetadataEnabled(t *testing.T) { | |
| func TestWithEndpointMetadataDisabled(t *testing.T) { |
| svc.Representatives.RepresentativeInfoByAddress().Do() | ||
|
|
||
| spans := mt.FinishedSpans() | ||
| assert.Len(t, spans, 1) |
There was a problem hiding this comment.
nit:
| assert.Len(t, spans, 1) | |
| require.Len(t, spans, 1) |
| svc.Representatives.RepresentativeInfoByAddress().Do() | ||
|
|
||
| spans := mt.FinishedSpans() | ||
| assert.Len(t, spans, 1) |
There was a problem hiding this comment.
nit:
| assert.Len(t, spans, 1) | |
| require.Len(t, spans, 1) |
| func downloadGoogleApiSrc() string { | ||
| zipUrl := downloadUrl() | ||
| func downloadGoogleAPISrc() (string, error) { | ||
| zipUrl := googleAPIClientURL() |
There was a problem hiding this comment.
ooops I missed this one before
| zipUrl := googleAPIClientURL() | |
| zipURL := googleAPIClientURL() |
| @@ -83,3 +85,11 @@ func WithAnalyticsRate(rate float64) Option { | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| // WithEndpointMetadataEnabled allows to enable/disable the enriched Google endpoint metadata behavior for | |||
There was a problem hiding this comment.
Let's add some details here on why a customer would want to pass this option to override the default of true (totally ok to just copy some bits from the warning)
There was a problem hiding this comment.
I tried to write a shorter version of the warning here, please let me know if that works!
It looks like all of your comments have been addressed. Would it be possible to get a re-review when you get a chance? |
What does this PR do?
google.golang.org/apiversion used in the project and re-generatecontrib/google.golang.org/api/endpoints_gen.gofile using the same version.Testing outlier google.golang.org/apiCI job to pin thegoogle.golang.org/apiversion instead ofgoogle.golang.org/grpc.WithEndpointMetadataDisabledoption tocontrib/google.golang.org/apito allow disabling the behavior of setting dynamicallyservice.nameandresource.namebased on success/failure of endpoint identification.Motivation
Trying to unblock #1889
Describe how to test/QA your changes
Reviewer's Checklist