Skip to content

Conversation

@khusseini
Copy link

Related Issue or Design Document

Implements #753

Checklist

  • I have read the contributing guidelines and signed the CLA.
  • I have referenced an issue containing the design document if my change introduces a new feature.
  • I have read the security policy.
  • I confirm that this pull request does not address a security vulnerability.
    If this pull request addresses a security vulnerability,
    I confirm that I got approval (please contact [email protected]) from the maintainers to push the changes.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added the necessary documentation within the code base (if appropriate).

Further comments

@khusseini khusseini changed the title feat: Added extraObjects to Oathkeeper chart feat: added extraObjects to Oathkeeper chart Mar 8, 2025
Copy link
Collaborator

@Demonsthere Demonsthere left a comment

Choose a reason for hiding this comment

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

Hello there!
I don't have any issues with the feature, but I see how we could reuse it easier in other charts. Please ass the extra-manifest as a template to the https://github.com/ory/k8s/tree/master/helm/charts/ory-commons/templates library and we can easily include it from there. This will make reusing this much simpler :)

@khusseini
Copy link
Author

@Demonsthere I moved the template to ory-commons helm chart templates folder. Sorry it took me a while to do this.

@khusseini khusseini requested a review from Demonsthere March 29, 2025 21:45
Copy link
Collaborator

@Demonsthere Demonsthere left a comment

Choose a reason for hiding this comment

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

a few things here, as this wont work this way. The ory-commons is a library chart, and doesnt have Values or manifests, but only functions like https://github.com/ory/k8s/blob/master/helm/charts/ory-commons/templates/_helpers.tpl#L4
You need to refactor the extra manifests into a template function, then add the library as a dependency to oathkeeper as in https://github.com/ory/k8s/blob/master/helm/charts/keto/Chart.yaml#L29
then you have access to the functions inside which can be called similar to https://github.com/ory/k8s/blob/master/helm/charts/keto/templates/deployment.yaml#L97

So in the end, you should have in oathkeeper the extra-manifests.yaml file with a structure similar to

{{- with .Values.extraObjects }}
{{ include "ory.generateExtraObjects" . }}
{{- end }}

@Demonsthere
Copy link
Collaborator

Hey there @khusseini no worries ;) I have written more details on how i think this change should be implemented. If you have any questions, please ping me 😉

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