Auth Wrapper #1174
Auth Wrapper #1174
Conversation
service.go
Outdated
| @@ -38,10 +39,14 @@ func newService(opts ...Option) Service { | |||
| options.Client = wrapper.FromService(serviceName, options.Client) | |||
| options.Client = wrapper.TraceCall(serviceName, trace.DefaultTracer, options.Client) | |||
|
|
|||
| // parse command line flags | |||
| cmd.Init() | |||
There was a problem hiding this comment.
@asim I needed to do cmd.Init so that the auth package would be initialised with the necessary flags, e.g. excluded endpoints, public key etc. I've tested this and it's working well, but it feels like this shouldn't be here. Can you think of any potential problems I may have missed?
There was a problem hiding this comment.
You cannot do this here. Flags are parsed when calling cmd.Init. This will violate the users expectation. When we do NewService our expectation is that this is just a clean simple initialisation which anyone can use without the flag parsing. If you do service.Init there is the knowledge that this will parse flags. It will also barf in the runtime anywhere we've used NewService. Since that execution occurs inside an existing cli parser.
It doesn't look like the auth wrapper needs those things outside of the interface so the assumption being when service.Init is called these things will be initialisation by config/cmd in the default auth interface. In the micro runtime this would end up being preinitialised since its already done that cmd.Init call.
There was a problem hiding this comment.
I thought this also, but when I tested it the auth was noop, despite adding the auth flag. I checked this again when the wrapper was called upon request, and the same was true then.
There was a problem hiding this comment.
Fixed. Unfortunately I can now remember why this problem occurs. We've moved from setting defaults to setting the values in the service itself. cmd.Init sets defaults and service.Init sets the values in the service itself.
I've fixed by creating a lazy loading function that returns the auth interface from service.options. It probably pushes us to clearly define what the expected behaviour is. Ideally service.Init should only set its only values rather than defaults in different packages and we should make use of our own internal values set in the options.
|
@asim thanks for fixing the |
|
LGTM will merge |
No description provided.