✨deploy-image/v1-alpha: add a todo comment for users stating envtest limitations #2859
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: camilamacedo86, NikhilSharmaWe 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
|
I'm sorry, when you say "move" do you mean move my code changes, or actually propose changes to the scaffold that fixes these issues (by making it possible to use several namespaces)? The TODO helps of course. It's important that users are aware of the limitation. 👍 But even better would be code that supported several tests. |
| @@ -60,6 +60,8 @@ var _ = Describe("Busybox controller", func() { | |||
| }) | |||
|
|
|||
| AfterEach(func() { | |||
| // TODO(user): Attention if you improve this code by adding other context test you MUST | |||
| // be aware of the current delete namespace limitations. More info: https://book.kubebuilder.io/reference/envtest.html#testing-considerations | |||
| By("Deleting the Namespace to perform the tests") | |||
| _ = k8sClient.Delete(ctx, namespace) | |||
|
|
|||
There was a problem hiding this comment.
HI @mogsie,
I'm sorry, when you say "move" do you mean move my code changes, or actually propose changes to the scaffold that fixes these issues (by making it possible to use several namespaces)?
I could not follow this one.
The TODO helps of course. It's important that users are aware of the limitation. 👍
But even better would be code that supported several tests.
Will we have in all case scenarios many tests?
OR in many cases to unit test the controller we might have only one test that just checks the reconciliation.
So, I think we can add the TODO here and ensure that it is still simple.
However, we can also add an example in the docs WDYT? Also, we probably need to vote on the C+R issue and see if we can address this one. The ideal solution for me shows we fix it in the C+R and we just let people know about that now. WDYT? Could we agree?
There was a problem hiding this comment.
I think the TODO approach is best. It allows the developers to make the choice:
- Either I keep within the single namespace, and clean up nicely after myself whenever I need to, and avoid problems like that
- Or I decide to make new namespaces for each new test, with the additional extra work needed.
Personally, I like having the ability to write integration tests and not have to worry about cleaning up, especially while developing, when I may not have completed the clean-up portion of the controller. In those cases, cleaning up is difficult. But it's not always needed, so I think the TODO comment is the best approach.
There was a problem hiding this comment.
That is terrific. So, let's move forward with this one since we have your :
/lgtm
k8s-ci-robot
added
the
lgtm
Description
Added a todo comment for users in controller_test.go file for deploy-image/v1-alpha plugin, highlighting the limitations of the envtest.
Motivation
Closes: #2856