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

🔧 First draft of helm #3

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

afreyermuth98
Copy link

PR about this issue :
#2 (comment)

Signed-off-by: afreyermuth98 <[email protected]>
@afreyermuth98
Copy link
Author

@eliebleton-manomano

Copy link
Contributor

@eliebleton-manomano eliebleton-manomano left a comment

Choose a reason for hiding this comment

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

That's a solid start. Thanks for volunteering :) I like that you made naming/namespacing configurable.

The cert-manager part seems to be missing, and our original cert-manager manifest appears to have been dropped. The hook cannot function without TLS enabled as far as I know. I'm not familiar with how folks deal with their PKI without cert-manager these days - so I don't know if it makes sense to support something else.

A nitpick: I'm not sure adding suffixes to resources names like -cluster-role or -role is idiomatic - even though it does happen in the wild - most resources are not named like that. After all, the resources do have a well defined type that transpires through kubectl get role/rolename for instance.

admissionReviewVersions: [ "v1" ]
sideEffects: None
failurePolicy: Ignore
timeoutSeconds: 2
Copy link
Contributor

Choose a reason for hiding this comment

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

I recommend a 1 second timeout here, and it could be worth making this customizable through values.

apiVersion: v1
kind: Pod
metadata:
name: "{{ include "knss.fullname" . }}-test-connection"
Copy link
Contributor

Choose a reason for hiding this comment

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

The knss abbreviation seems to only be in use here and not part of _helpers as they are.

Copy link
Author

Choose a reason for hiding this comment

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

lol yeah I first created the name chart with the name knss and then renamed it because i didn't like it. I'll correct that ^^

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that deletion may have been an accident.

Copy link
Author

Choose a reason for hiding this comment

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

of course

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

Successfully merging this pull request may close these issues.

2 participants