search

argoproj-labs / argocd-operator

A Kubernetes operator for managing Argo CD clusters.
https://argocd-operator.readthedocs.io
Apache License 2.0
658 stars 772 forks source link

feat: applicationset volumes and volumemounts spec #1563

Closed appiepollo14 closed 1 month ago

appiepollo14 commented 1 month ago

What type of PR is this?

/kind enhancement

What does this PR do / why we need it:

To be able to add extra volumes and volumeMounts to the appSet deployment, see related issue.

Have you updated the necessary documentation?

Documentation update is required by this PR. Documentation has been updated. Which issue(s) this PR fixes:

Fixes https://github.com/redhat-developer/gitops-operator/issues/620

How to test changes / Special notes to the reviewer:

appiepollo14 commented 1 month ago

@svghadi can you please help me with the following questions: a. Is there a need to write e2e-tests for this quite trivial change. I've written unit tests for it. b. If e2e-tests are required, I've added a starter, can you explain why it if failing. Check the CI-logs. How is it possible that the modified applicationSet api, is not available in the e2e tests. Is this by design? Can't a api change be accompanied with a new e2e-test in the same PR? Please give me some pointers.

see newly created issue https://github.com/argoproj-labs/argocd-operator/issues/1566 for running this locally. Would be very handy to be able to test the kuttl tests locally. Would decrease a cycle with 45 mins in this case.

Edit: I've come to the conclusion that I've was using the v1alpha1 api in the e2e-test and changed the v1beta1 api. Which raises my question. Is it OK to only change v1beta1 spec?

appiepollo14 commented 1 month ago

@svghadi ready for your approval!

svghadi commented 1 month ago

a. Is there a need to write e2e-tests for this quite trivial change. I've written unit tests for it.

Well, it depends on the feature. Personally, I try to add e2e tests for every feature. However, sometimes it can be tricky, so exceptions can be made.

Edit: I've come to the conclusion that I've was using the v1alpha1 api in the e2e-test and changed the v1beta1 api. Which raises my question. Is it OK to only change v1beta1 spec?

Yes. v1alpha1 is deprecated so changes to v1beta1 only are fine.

appiepollo14 commented 1 month ago

Left a suggestion, rest all looks good.

Also, it would be good to have a unit or e2e test to check default vols & mounts when additional vols & mounts are not set/removed from ArgoCD CR.

There already is, each test calling: https://github.com/appiepollo14/argocd-operator/blob/feat/appsetcontroller-extra-volume-mounts/controllers/argocd/applicationset_test.go#L106 checks the default vols * mounts.

svghadi commented 1 month ago

Okay. Looks good then. Can you fix the merge conflicts?

appiepollo14 commented 1 month ago

Okay. Looks good then. Can you fix the merge conflicts?

Fixed!