search

eshepelyuk / cmak-operator

CMAK (prev. Kafka Manager) for Kubernetes
MIT License
59 stars 27 forks source link

Allow the user to pass podAnnotations to the underlying job #42

Closed jack1902 closed 2 years ago

jack1902 commented 2 years ago

This enables the cmak-reconcile jobs spun up to have additional podAnnotations passed. This is especially useful in a namespace which is being automatically injected by a service-mesh but is undesirable on jobs which are finite (linkerds sidecar will run forever resulting in the cmak-reconcile pod never dying if injected).

eshepelyuk commented 2 years ago

Hello @jack1902 thanks for a contribution.

jack1902 commented 2 years ago

@eshepelyuk unsure of how to run the tests locally for this project, but I have updated from podAnnotations to annotations as requested.

The annotations are additive but won't break backwards compatibility so in theory the tests that exist now should still function unless they are string comparisons?

EDIT:

NVM, I can see ./tests/linter/test.sh will address any failures.

EDIT Two: I'm seeing numerous errors but I can see that these are potentially expected, however the ingress object within the chart fails against the kubernetes version I have since [WARNING] templates/ingress.yaml: networking.k8s.io/v1beta1 Ingress is deprecated in v1.19+, unavailable in v1.22+; use networking.k8s.io/v1 Ingress

eshepelyuk commented 2 years ago

@jack1902 -

  1. check CI / CD pipeline how to run linter and unit tests locally https://github.com/eshepelyuk/cmak-operator/blob/master/.github/workflows/ci.yaml#L21-L24
  2. When running linter tests locally - ignore the output, just analyze final return code - must be 0
jack1902 commented 2 years ago

@eshepelyuk added some tests around the annotation addition and the stuff is passing image

jack1902 commented 2 years ago

@eshepelyuk I've added additional annotations for the job-setup created via a helm hook as this caught me off guard when deploying this locally to my environment. I've added additional tests to cover this as it was easy enough once I understood how the helm unittest plugin worked

jack1902 commented 2 years ago

@eshepelyuk i've used the single annotations now for both jobs and added tests for this work which cover your point 3 since annotations are only added if they exist otherwise metadata doesn't exist (which is desirable for the chart as it wasn't present previously)

eshepelyuk commented 2 years ago

Well, point by point

  1. rollback file test/unit/reconcile-placement.yaml
  2. merge test/unit/reconcile-annotations.yaml and test/unit/setup-annotations.yaml into a new file test/unit/job-annotations.yaml
  3. in test/unit/job-annotations.yaml both for cron job and for job manifests add test cases when you don't pass any annotaitons to values, i.e. case by default

@jack1902 plz follow this exactly as written

jack1902 commented 2 years ago

merged the files and added a new set of annotations to the deployment which are also covered by tests.

jack1902 commented 2 years ago

@eshepelyuk ive got the changes in this for the deployment, if you are happy with all the changes here, i can open a second PR to merge the deployment changes too.

eshepelyuk commented 2 years ago

@jack1902 please read all comments cafefully and apply all of the requested changes, exactly as written.

jack1902 commented 2 years ago
  1. rollback file test/unit/reconcile-placement.yaml - no longer shows up in the diff
  2. merge test/unit/reconcile-annotations.yaml and test/unit/setup-annotations.yaml into a new file test/unit/job-annotations.yaml - completed
  3. in test/unit/job-annotations.yaml both for cron job and for job manifests add test cases when you don't pass any annotaitons to values, i.e. case by default - this is exactly what is covered by the should not have annotations by default test, please elaborate what is missing with a useful contribution or example as I am confused as to what you mean if this is not the case.

I have dropped the deployment annotations entirely.

jack1902 commented 2 years ago

Completed, i can't merge the two commits on my mobile so I'll have to do this tomorrow if this is a blocker for the pr merging

eshepelyuk commented 2 years ago

Completed, i can't merge the two commits on my mobile so I'll have to do this tomorrow if this is a blocker for the pr merging

Ahh ok z i ll do it

eshepelyuk commented 2 years ago

@jack1902 thnx for contrivution. I ll merge this PR and i am ready to new one regarding the deployemnt annotations.

Then i ll create a new release. Makes sense ?

jack1902 commented 2 years ago

Makes sense, thanks for the merge. The annotations for the deployment don't matter as much, since the annotations I needed actually had to go on zookeeper to enable my deployment of Kafka to work.

The annotations to disable service mesh injection on the jobs was the main blocker I originally ran into

eshepelyuk commented 2 years ago

Well, then I am making a release now.