search

meraki / dashboard-api-go

Dashboard API for Golang
MIT License
19 stars 7 forks source link

retryClient does not expose logger or retry configuration #6

Closed stephengroat-dd closed 1 year ago

stephengroat-dd commented 1 year ago

https://github.com/meraki/dashboard-api-go/blob/0ae3ac4255037d478b7ff8f12b2d8db9a9268cd2/.openapi/templates/client.mustache#L65-L67

Currently, the retryClient does not expose the logger, causing DEBUG level logs to always be emiited.

Exposing other parts of the retryclient would also be good to allow for custom retry configurations (if needed)

iamdexterpark commented 1 year ago

Hey @stephengroat-dd Thanks a lot for opening this issue. We really appreciate your feedback and code review of the repo.

I wanted to let you know that we're aware of the issue you mentioned regarding the logger not being exposed in the retryClient. It's definitely something we plan to address as part of our upcoming upgrade to the codebase. This will also allow for custom retry configurations, as you suggested.

Although we don't have a specific timeline for this yet, it's definitly next steps in terms of priorities for this client. We will keep you updated on our progress.

Once again, thanks for your input, and please feel free to reach out if you have any further questions or suggestions.

Thank you for helping to make this repo even better! 😄

ryan-berger commented 1 year ago

Hey @stephengroat-dd , I'm a teammate of @iamdexterpark and I've been tasked with taking care of this as Dexter is quite busy.

I'm thinking that rather than exposing retry logic to consumers, it would be best to make an equivalent to the stdlib's net/http.DefaultClient be the default HTTP client. We made the retry client the default HTTP client if no other client is passed in because we need retries, but most people likely won't.

If you would like to configure retries, then you'll need to import the retryhttp library yourself and configure it that way. Then you have full control over the configuration without adding extra API surface to this client.

Since we dogfood this it also means that tearing this out will require us to do the same, so I don't think it is too much work.

I'll open a PR here shortly with my changes. (Edit: it is open, see #13 )

iamdexterpark commented 1 year ago

Hello @stephengroat-dd,

I've merged Ryan's changes (thank you @ryan-berger) into a new release that also leverages the Meraki OpenAPI v3 specification.

I'll mark this issue as resolved but please let us know if you notice any other issues. Again, thank you for the input, and please feel free to reach out if you have any further issues, questions or suggestions.