credentials/alts: Update ALTS "New" APIs #1921
Conversation
cesarghali
added
the
Type: API Change
There was a problem hiding this comment.
This seems fine in general, and provides a good way of extending the API in the future without breaking existing users.
I just noticed there is a flag defined in this package. Flags in a non-main package are to be avoided if at all possible. I would recommend removing it ASAP.
credentials/alts/alts.go
Outdated
| } | ||
|
|
||
| // DefaultClientOptions creates a default (empty) ClientOptions. | ||
| func DefaultClientOptions() *ClientOptions { |
There was a problem hiding this comment.
This should not be necessary; your zero-value defaults should all be sane (and I see you do end up with the zero value, so this is true).
| type ClientOptions struct { | ||
| // TargetServiceAccounts contains a list of expected target service | ||
| // accounts. | ||
| TargetServiceAccounts []string |
There was a problem hiding this comment.
Is this always (or nearly always) supposed to be non-empty? Or is it common to omit it?
If it's commonly used, it probably should be left as a parameter.
There was a problem hiding this comment.
This is not commonly used. I'm expecting callers will set this in very few situations.
cesarghali
force-pushed
the
alts_new_client_api
branch
from
7b15bb1
to
0faf247
Compare
cesarghali
changed the title
|
Thanks for the comments Doug. Regarding the flag, there was a discussion about it in #1865. I'm happy to discuss it more offline and we do further changes in a separate PR. |
Change the
alts.NewClientAPI to take a struct containing all client-side options instead of passing them using parameters. This allows adding more options in the future without breaking the existing API or having to create a new one.This API also changes
alts.NewClientandalts.NewServertoalts.NewClientCredsandalts.NewServerCreds, respectively.This API is only used by the
interop/alts/client/client.go.