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

feat: add authMethod to kratix chart #8

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

Conversation

Knapsack44
Copy link

I seem to have messed something up due to the fact that my email address was missing from the commit. This meant that the CLA could not accept correctly. Hopefully everything should be fine now.

Original PR:
This PR will add the possibility to define the authMethod for the Kratix helm chart. If no value is defined, the value is automatically set to basicAuth. With my change, this will also be explicit visible.

@ChunyiLyu
Copy link
Member

ChunyiLyu commented Sep 12, 2024

@Knapsack44 Thanks again for contributing.

We support two different StateStore that has different defaults. For our GitStateStore, default authentication method is basicAuth, for BucketStateStore, default is accessKey. The team think it's better to leave defaulting to the controller as it's already been done in:
https://github.com/syntasso/kratix/blob/main/api/v1alpha1/bucketstatestore_types.go#L40
https://github.com/syntasso/kratix/blob/main/api/v1alpha1/gitstatestore_types.go#L44

There's definitely value in allowing setting authMethod for the chart. May I suggest modify the PR to define the authMethod and remove the part about setting default?

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