k8up-io / k8up

Kubernetes and OpenShift Backup Operator
https://k8up.io/
Apache License 2.0
617 stars 63 forks source link

Added service account annotations #876

Closed mkotsalainen closed 1 year ago

mkotsalainen commented 1 year ago

Summary

Helm chart service account annotation. Needed for EKS IRSA roles. I took code from: https://github.com/external-secrets/external-secrets/blob/main/deploy/charts/external-secrets/values.yaml#L78 https://github.com/external-secrets/external-secrets/blob/main/deploy/charts/external-secrets/templates/serviceaccount.yaml#L12

mkotsalainen commented 1 year ago

Here's related issue: https://github.com/k8up-io/k8up/issues/875

mkotsalainen commented 1 year ago

The comment for the parameter is wrong.

Good catch! Fixed.

Could you please also run make chart-docs and git rebase master --signoff on your branch to make our checks pass.

Done.

Kidswiss commented 1 year ago

hmmm... the DCO check is still not happy.

Can you try this way?

git rebase HEAD~6 --signoff
git push --force-with-lease origin sa-annotation
mkotsalainen commented 1 year ago

hmmm... the DCO check is still not happy.

Can you try this way?

git rebase HEAD~6 --signoff
git push --force-with-lease origin sa-annotation

done

mkotsalainen commented 1 year ago

is there something more I need to do on this?

Kidswiss commented 1 year ago

Sorry I was off pretty early yesterday.

The commits look a bit weird now. Did you maybe rebase or squash before you executed my signoff command? It also signed commits from other people with your mail address.

Could you please drop those from the PR?

mkotsalainen commented 1 year ago

If I'm not mistaken executed the commands in the order that they appeared in the chat. To tell you the truth I didn't really know what I was doing - I've never seen the --force-with-lease flag before :)

Anyways, I've tried but I can't seem to get rid of those older commits from my branch.

If you want I can try to fork your repo again and redo from scratch, maybe that helps? Or you just take my diff and merge it - I don't care about attribution - I just want to be able give an AWS IRSA role to the k8up pod so that we can use it.

Kidswiss commented 1 year ago

Yeah the whole DCO thing is a bit fiddly unfortunately... But it's required as we're a CNCF project.

I have a look to get this merged and released. Maybe I can cherry-pick your commits onto a new branch.

Kidswiss commented 1 year ago

I've merged it via #877.

I just triggered a release, it should be available in a few minutes.

mkotsalainen commented 1 year ago

Thanks!!

mkotsalainen commented 1 year ago

Sorry to bother you with this again but there is a whitespace in front of annotations:

2023-08-04 at 21 28

This causes the helm chart to render a non-valid ServiceAccount spec. Helm / yaml is picky.

This indentention level works:

{{- if .Values.serviceAccount.create -}}
apiVersion: v1
kind: ServiceAccount
metadata:
  name: testing
  {{- with .Values.serviceAccount.annotations }}
  annotations:
    {{- toYaml . | nindent 4 }}
  {{- end }}
{{- end }}
2023-08-04 at 21 32
Kidswiss commented 1 year ago

No worries, I should have caught that during the review 🙈. The git mess made me miss that, sorry. I'll create a fix for that.