deployment CR reconcile of containers

[Boomatang]

Ensure that only containers that are managed by the limitador-operator can be added to the limitador deployment CR.

Verification

• Run this branch

make local-setup

• Add a limitador deployment

echo "
   apiVersion: limitador.kuadrant.io/v1alpha1
   kind: Limitador
   metadata:
     name: limitador-sample
   spec:
     listener:
       http:
         port: 8080
       grpc:
         port: 8081
     limits:
       - conditions: ["get_toy == 'yes'"]
         max_value: 2
         namespace: toystore-app
         seconds: 30
         variables: []
   " | kubectl apply -f -

• Check: There should only be one container in the deployment spec

kubectl get deployment limitador-sample -o jsonpath='{.spec.template.spec.containers}' | jq 'length'

• Add new container to the deployment CR

kubectl patch deployment limitador-sample -n default --type='json' -p='[{"op": "add", "path": "/spec/template/spec/containers/-", "value": {"name": "hello-sidecar", "image": "quay.io/podman/hello"}}]'

• Expected: The patch will be complete and kubectl will not return an error. But the operator will revert the changes to only have one container present.

kubectl get deployment limitador-sample -o jsonpath='{.spec.template.spec.containers}' | jq 'length'

Resolves #90

[KevFan]

Changes looks good 👍

Any thoughts on adding a controller test for this also? 🤔

• https://github.com/Kuadrant/limitador-operator/blob/main/controllers/limitador_controller_test.go#L244-L283

[KevFan]

This looks good to me 👍

[eguzki]

LGTM

however one question before giving my approval.

So far, we have been using Ginkgo for e2e tests in controllerspackage. For the remaining packages we have been using Golang builtin testing framework. To be honest, I do not remember the rationale for that. Is there any drawback you can think about breaking this internal convention?

[eguzki]

nitpick: this approach is not wrong perse. However I find less robust than defining a function to return the object

func myFancyDeployment() *appsv1.Deployment {
    return &appsv1.Deployment{ 
             ....
    }
}

"Robust" meaning less prone to errors like forgetting to set the new value or being nil for whatever reason.

[Boomatang]

I don't see this to be less robust. The beforeEach ensures the desired deployment is set before each test context and a test context has to actively change the deployment if the test context requires some else. I find if there is a function that returns a default set up (deployment in this case) people tend to start adding logic to the function to handle different cases which kinda makes it pointless to have a default.

I am not sure what the ask is here. If you want me to rewrite the test to suit go test as in the other comment this would be gone so nothing is needed. But if the rewrite is not need I am not sure what change you are asking me to do.

[eguzki]

no changes requested. Just a comment to refactor to something else. Since you do not agree with me, as I said this is not wrong, thus nothing to be done here.

[Boomatang]

If ginkgo is only to be used for e2e tests I don't mind switching the golang built in test framework. When it comes to the drawbacks of using one over the other. Personal I like ginkgo. Being able to configure the beforeEach and afterEach on at every level is great and allows for clean tests as the test set ups do not need to be part of the test case. Using Gomega which goes hand in hand ginkgo, puts a clear understanding in the test case but of course Gomega can be used as part with the standard test frame work. Some other features of ginkgo that super usefully is being able to randomize not only the test suites but all the tests with a suite. This can highlight flaky tests. There is a also the retry function that can be added to know flaky tests. Being able to build a binary of the test suite would be more usefully for sharing e2e test images with qe. I will mention that Ginkgo does allow running test in parallel, while this can be done in gotest it does require code changes.

So then why use go test. There is better support for go test in IDEs. For instances goland can generate the test skeleton for a function which the user only has to add test case to. This make is super easy to get unit test up and working. But the biggest advantage is it is part of the language.

I am all for using one style. @eguzki not sure if I answered your question. Do you want me to rewrite the test I created to use the go test framework?

[eguzki]

let's go with this. 🚀