-
Notifications
You must be signed in to change notification settings - Fork 2
Implement multigateway controller #36
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
Conversation
c7f1cba to
2b876ce
Compare
rytswd
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cannot approve this if it has NO test code. I believe we should be able to get near 100% (except for the part that needs envtest setup), and at least I need to see some confirmation that the code does what it's supposed to do.
|
@rytswd I'm very keen to get started on increasing the test coverage and gaining confidence in the code we write. Though I believe that at this stage in the project, it's more pressing to get something running untested than something which doesn't run but is ~100% tested. I wrote in the description that the Would you agree? Keen to hear your thoughts. |
|
I see your point, and probably where you are coming from. I am guessing you are keen to get some sort of complete picture as early as we can, so that we know more about what else we need to do. (Correct me if my assumption is inaccurate!) However, I disagree with that. This is not a project of rapid trial and error cycle. There are well known patterns we can follow to create an operator, and currently our main target is to handle standard Kubernetes objects. There are no uncertain factors as of now AFAIK, and having test cases makes it much easier for reviewers to be confident about the code does the right thing. I would even push for 100% test coverage early on. The later we wait, the more difficult it would become. Once we have 100% test coverage, it is easier to request 100% for any code changes. Also, how you mentioned about the Deployment to not function fully is interesting -- because that's precisely the reason why I want to see the unit test code to give us the confidence that the operator logic is working correctly. Btw, the operator container is pretty simple to build and run in a test cluster (you can do that with #45). As I said, because this is a well-known pattern of operator work, I do not particularly care about that at the moment. There will be a lot more of fine-tuning we need to do for the image + binary. We would surely need to work on that, but it's much later down the line, when we start testing with real clusters to deploy. |
2b876ce to
6ec9294
Compare
6ec9294 to
97441bd
Compare
|
@rytswd understood and thank you for taking the time to explain your position. I'm surprised, because you were quite okay with #25 being merged without tests just two days ago, and in your words:
I think it's naive to think that there are no uncertainties - I will always prefer to iterate and validate, but I've done what you requested - the tests are now in place. |
numtide/multigres-operator:latestpkg/resource-handler/controller/etcdso that single-container deployments/statefulsets/daemonsets use consistent words