Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added service account annotations #876

Closed
wants to merge 16 commits into from

Conversation

@mkotsalainen mkotsalainen requested a review from a team as a code owner July 31, 2023 12:12
@mkotsalainen mkotsalainen requested review from tobru and glrf and removed request for a team July 31, 2023 12:12
@mkotsalainen
Copy link
Author

Here's related issue: #875

charts/k8up/values.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

@Kidswiss Kidswiss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment for the parameter is wrong.

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

@mkotsalainen
Copy link
Author

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
Copy link
Contributor

Kidswiss commented Aug 2, 2023

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

glrf and others added 12 commits August 2, 2023 13:26
Signed-off-by: Fabian Fischer <[email protected]>
Signed-off-by: Matti Kotsalainen <[email protected]>
Signed-off-by: Fabian Fischer <[email protected]>
Signed-off-by: Matti Kotsalainen <[email protected]>
Signed-off-by: Fabian Fischer <[email protected]>
Signed-off-by: Matti Kotsalainen <[email protected]>
We enabled vale linting for documentation on CI. Vale found a couple of
issues, that we fix with this commit.

Signed-off-by: Fabian Fischer <[email protected]>
Signed-off-by: Matti Kotsalainen <[email protected]>
Signed-off-by: Tobias Brunner <[email protected]>
Signed-off-by: Matti Kotsalainen <[email protected]>
Signed-off-by: Tobias Brunner <[email protected]>
Signed-off-by: Matti Kotsalainen <[email protected]>
Signed-off-by: Tobias Brunner <[email protected]>
Signed-off-by: Matti Kotsalainen <[email protected]>
Signed-off-by: Tobias Brunner <[email protected]>
Signed-off-by: Matti Kotsalainen <[email protected]>
Signed-off-by: Tobias Brunner <[email protected]>
Signed-off-by: Matti Kotsalainen <[email protected]>
Signed-off-by: Matti Kotsalainen <[email protected]>
Signed-off-by: Matti Kotsalainen <[email protected]>
Signed-off-by: Matti Kotsalainen <[email protected]>
@mkotsalainen
Copy link
Author

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
Copy link
Author

is there something more I need to do on this?

@Kidswiss
Copy link
Contributor

Kidswiss commented Aug 3, 2023

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?

Signed-off-by: Matti Kotsalainen <[email protected]>
Matti Kotsalainen added 3 commits August 3, 2023 18:15
Signed-off-by: Matti Kotsalainen <[email protected]>
Signed-off-by: Matti Kotsalainen <[email protected]>
@mkotsalainen
Copy link
Author

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
Copy link
Contributor

Kidswiss commented Aug 4, 2023

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
Copy link
Contributor

Kidswiss commented Aug 4, 2023

I've merged it via #877.

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

@Kidswiss Kidswiss closed this Aug 4, 2023
@mkotsalainen
Copy link
Author

Thanks!!

@mkotsalainen
Copy link
Author

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
Copy link
Contributor

Kidswiss commented Aug 7, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants