Skip to content
This repository has been archived by the owner on Oct 21, 2020. It is now read-only.

NewProvisionController needs refactoring #77

Closed
wongma7 opened this issue Apr 18, 2017 · 3 comments · Fixed by #80
Closed

NewProvisionController needs refactoring #77

wongma7 opened this issue Apr 18, 2017 · 3 comments · Fixed by #80

Comments

@wongma7
Copy link
Contributor

wongma7 commented Apr 18, 2017

The parameters could be documented better, and we are adding new ones relatively often. @xingzhou I have started a little work on implementing this functional options pattern, wdyt? https://dave.cheney.net/2014/10/17/functional-options-for-friendly-apis . We are not dealing with a complex API here, just one function, but anything to make it more pleasant to use is good :).

@xingzhou
Copy link
Contributor

Thanks @wongma7 for creating the ticket. Something in my mind, for discussion:

  1. Shall we keep the old NewProvisionController API for a while? As a external used project, I believe there should be quite some users started to use this controller in their own provisioner, if we can keep the old NewProvisionerController method, this may provide some time for the users to change their code.

  2. For those options we want to pass into the construction of controller, do we support to change those options during controller's runtime? Because for some retry count related options, if we can dynamically change them during the controller runtime, I think it would provide some benefits for user's provisioner code.

Once the new constructor is ready, I think we can inform the community in the mailing list about the changes, and I can help provide the patches for local path/NFS provisioners to adopt the new constructor.

@wongma7
Copy link
Contributor Author

wongma7 commented Apr 20, 2017

  1. I am leaning toward doing this: release 2.1.0 soon with the deletion retry threshold changes and mark NewProvisionController deprecated. But then I don't think we need to wait long, we could bite the bullet and release 3.0.0 shortly afterward where the only change is the new, less ugly, NewProvisionController. Better to break it sooner than later IMO, then hopefully we can stay on 3.x.x for a longer while, truly respect semver, and not annoy consumers so much with the periodic changes to NewProvisionController's signature.
  2. Yes, we could, I guess we have to decide which are changeable and which not. Thresholds being changeable make sense to me

Apologies in advance to anybody using the lib and annoyed by breaking changes to the API -- which is yes, only one function, but still annoying I'm sure :).

@wongma7
Copy link
Contributor Author

wongma7 commented Apr 25, 2017

btw, I think I'll wait for client-go 3.0.0 before releasing our own 3.0.0. Client-go broke their API again, giving me another excuse to break ours in the next month or so.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants