Create default NGINX API Client with specified Timeout #124
Conversation
jjngx
added
enhancement
github-actions
bot
added
the
dependencies
|
|
||
| // WithAPIVersion configures NGINX Plus Client to use a specific version of the NGINX Plus API. | ||
| // | ||
| //nolint:all |
There was a problem hiding this comment.
I think I'm struggling to see why these functions are needed and why they can't be "normal" parameters of NewDefaultNginxClient. Also I think we should avoid adding new code that needs a //nolint:all.
There was a problem hiding this comment.
I think I'm struggling to see why these functions are needed and why they can't be "normal" parameters of
NewDefaultNginxClient. Also I think we should avoid adding new code that needs a//nolint:all.
Functions in question (functonal options pattern) provide ability to configure created obj by the user of the library. At the same time if user is happy to use a default obj, he is not forced to do not necessary paperwork (creating httpClient to pass it to the func to create the obj, passing explicit api version, etc.). This way the API is cleaner and function signature keep to a minimum. In other words, user can create a minimum usable obj with minimum paperwork involved from his side. If the user wants to pass his own params (in this case http client and version) he can do it with functional options.
Commenting on the //nolinit:all directive:
The linter we use (not staticcheck) issues a message that exported func returns a not exported type. This is intentional as the WithXYZ functions validates params passed to option func. the option type is used internally and is not exported, as the library user won't use it. The golangci-linter we use (with settings) for some reason emits a message. This message would make CI pipeline fail. So, the reason to keep this directive is to allow CI to pass.
There was a problem hiding this comment.
Thanks for the explanation. I understand the functional options pattern, I'm just not sure about adding a new NewDefaultNginxClient with just a couple of options, maybe we should do a refactor of the package.
We already have NewNginxClientWithVersion as well where you can pass the version, if anything I'd follow this pattern.
for the unexported-return error, what I usually see is that the options are private but the func is exported, see https://github.com/golang/text/blob/master/secure/precis/options.go#L16 for example.
There was a problem hiding this comment.
The option type is un-exported for a reason. It is used internally in the package and it is not a part of package API. External consumers of the library use WithXYZ functions to configure the client. The main point of this change is to have an option to initialise the NGINX client WITHOUT requiring unnecessary paperwork from user perspective. Chance the NewXYZ func returns a default client configured out of the box. If user WANTS to pass config params it has an option to do so.
The problem that we have with golangci lint is that it complains unnecessarily and we need to suppress this complaint. Otherwise the CI configured currently won't pass. Please not that staticcheck does not complain about exported functions returning not exported types.
The additional functionality of the new func that creates an instance of the NGINX client is to decouple the client from a running NGINX server the client tries to connect to.
We can rename the NewXYZ client however we wish to suit better naming convention we use in the library.
There was a problem hiding this comment.
I understand what the functionality is about, I just don't agree with the implementation.
I don't think golangci-lint is complaining unnecessarily 🤷♂️
Maybe somebody else from the team can weigh in @nginxinc/kic
| myHTTPClient := &http.Client{ | ||
| Timeout: 5*time.Second | ||
| } | ||
| c, err := NewNginxClient(myHTTPClient, "your-api-endpoint") |
There was a problem hiding this comment.
Sometimes it's client.New...() (see line 63 for example), sometimes it's just New...() (as in here). Can we keep this consistent?
| nc := NginxClient{ | ||
| apiEndpoint: apiEndpoint, | ||
| httpClient: &http.Client{ | ||
| Timeout: 10 * time.Second, |
There was a problem hiding this comment.
Is hard-coding a 10 second timeout here really creating a 'default' nginx client? I would have thought leaving the http.Client timeout it its default is more correct. If the user wants to specify 10 seconds then they can do that with WithHTTPClient()
At the least should we define the timeout with the rest of the constants towards the top of the file, and not use the magic number 10 here?
Proposed changes
http.Clientconfigured with 10s timeout.Checklist
Before creating a PR, run through this checklist and mark each as complete.