-
Notifications
You must be signed in to change notification settings - Fork 22
WIP: Server config via values + plugin config #49
Conversation
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
…ged with values. Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
8dee88b
to
6654998
Compare
…e/helm-charts into json-server-config-plugins Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
Rebased. |
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of small questions/suggestions? By moving those macros to helpers, I hope it becomes easier to see the diff on the configmap itself. It might make sense to split the configmap from the plugins part to have a smaller PR.
} | ||
} | ||
{{- end }} | ||
{{- define "spire-server.config-plugins-template" }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we define these functions in _helpers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I debated that. If I did, none of the config would be in the configmap template anymore. Which may be confusing to some reviewers? But I'm good either way honestly. _helpers.tpl would be good. or I could do a _configmap.tpl ?
port = 9988 | ||
} | ||
} | ||
{{- define "spire-server.config-main-template" }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we define these functions in helpers?
country: NL | ||
organization: Example | ||
common_name: example.org | ||
config: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to introduce this config level?
In the end everything we define here is config…
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Its working because it can toYaml all of .config. if we did all of the stuff at root, .Values, there would be a bunch of junk that leaks through.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For clarification, It allows any setting in the server block to be configured. So it solves overriding more then just the predefined stuff. We could template it all back out, but would loose the ability to set arbitrary settings within the server block.
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
Was asked to move this here for now: Lets please move the discussion there. |
This patch enables all of the server settings to be configured via values along with adding any plugin you want via values.
Everything is managed via yaml templating and is converted to json to be passed to spire in the end.