-
Notifications
You must be signed in to change notification settings - Fork 135
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
avoid package globals #3
Comments
Agreed. Lot's of refactoring is in order. This was a weekend project that ended up growing a bit larger than expected and may turn out to be generally useful. The next couple of weeks will be about cleaning up the docs and improving code quality. I'll also be deciding if this will grow into a true open source project or serve as a working example of how to build custom Kubernetes controllers. Today I'm happy if people can learn something from this repo and I encourage people to fork it and built sometime more awesome. |
@groob Also thanks for taking the time to review the code and file this issue. I'll keep this open as a promise to address your feedback over the next few weeks as part of a larger code review and clean up process. |
Thanks for the great example project, @kelseyhightower. We have a use-case that requires cert-manager to monitor all (save perhaps a blacklisted set) namespaces for certificate objects. Supporting this use case probably adds a lot of complexity that reduces the educational value of codebase. In your eyes, is this a fork situation or is the feature in question compelling enough to be worth integrating? |
this package relies on several global variables(example http.Client). For testing and maintainability, it would be better to refactor the code that relies on global state.
The text was updated successfully, but these errors were encountered: