Upgrade DNSimple client and add support for contexts #1551
Conversation
Use the branch, as we'll have to make a change to export again returned values.
k8s-ci-robot
added
cncf-cla: yes
weppos
changed the title
|
/assign @hjacobs |
provider/dnsimple.go
Outdated
| } | ||
|
|
||
| // dnsimpleZoneServiceInterface is an interface that contains all necessary zone services from dnsimple | ||
| // dnsimpleZoneServiceInterface is an interface that contains all necessary zone services from DNSimple | ||
| type dnsimpleZoneServiceInterface interface { | ||
| ListZones(accountID string, options *dnsimple.ZoneListOptions) (*dnsimple.ZonesResponse, error) |
There was a problem hiding this comment.
can we drop this same as ListRecords? seems to be the same method as without context
you might also can cleanup the test file
There was a problem hiding this comment.
Just to clarify, do you want me to remove the non-context ListRecords and ListZones methods?
In case, should I actually drop the WithContext versions (ListRecordsWithContext) and simply add the context in the existing method signature?
There was a problem hiding this comment.
Preferring the second option here and yes this would be perfect 👍 , just use the context in the existing method signature 🙂 .
Just keep the ones with the context.
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: njuettner, weppos The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
This PR upgrades to DNSimple client 0.60.0, and adds support for contexts. The latest client version accepts contexts.
Regarding contexts, I saw that methods connected to Kubernetes API already supplied contexts, so no signature change was required. However, the provider exposed a number of DNSimple-oriented custom methods which I suppose are helpers that are only used directly. I see other providers have similar helpers available. Since these methods were exported (hence could not be considered internal), I decided to not change the signature and add new methods
-WithContext.I would be happy to remove them and update the existing ones to always pass contexts, but I need some guidance whether this type of change is accepted.
Besides this note, I think the changeset is ready for review and all specs PASS.