-
Notifications
You must be signed in to change notification settings - Fork 22
Move spire-lib macro's to their own library chart #346
base: main
Are you sure you want to change the base?
Conversation
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.
LGTM
- name: spire-lib | ||
repository: file://./charts/spire-lib | ||
version: 0.1.0 |
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.
This is not a standard pattern. I suggest putting this in the parent chart. https://helm.sh/docs/chart_best_practices/templates/#names-of-defined-templates
This set of subchart would be improved by collapsing into one chart.
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.
This is not a standard pattern. I suggest putting this in the parent chart. helm.sh/docs/chart_best_practices/templates/#names-of-defined-templates
This set of subchart would be improved by collapsing into one chart.
Agree, we should hold this off till we break out some charts like spire-server etc. To not waste time keeping this in sync probably better to recreate this PR later. Instead of nesting this chart it can then be a top level chart with its own release cycle.
Before we can breakout charts we first need to fix #324 so our CI can support testing multiple charts.
@drewwells what we are aiming for is something like this. https://github.com/sigstore/helm-charts this will allow us to do advanced usecases like nested spire and such more flexible.
This spire-lib chart would be similar to the sigstore-common chart to share some commonly.
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.
This was an attempt to get the library chart done so the pr that breaks out the subcharts to /charts can have as minimal changes as possible. adding all this code into that breakout pr just makes for a lot more possible merge issues. Its not ideal to have under charts/spire/charts, but is a piece to getting it broken out.
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.
The breakout can be done chart by chart. Where the first breakout can be these lib functions. Next can be spiffe-csi, because that has very few dependencies, then probably spiffe-oidc.
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.
When I tried it last, it gets confused doing a helm dep up when you have subcharts directly in the charts/spire/charts/ dir. It tries to tar up each of the dependencies and then you get duplicates of the subcharts. I don't think having charts directly under another chart is actually officially supported by helm. So, I think all the charts need to be broken out all at once.
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 agree, this is not a standard pattern.
I'm all for supporting sub-charts, but there should be better dividing lines. Claiming that an Agent is a sub-chart doesn't make sense, as it needs to coordinate strongly with a Server, possibly an SPIRE CSI component, and maybe a controller manager.
For those items, putting them into the top-level spire chart makes sense, as these are all core components of SPIRE. For the other items like Tornjak, possible co-database deployment, etc. Those are not (in my mind) core SPIRE components, and using sub-charts makes a lot of sense.
@marcofranssen I understand that you are attempting to clone sigstore structure into this project; but, sigstore is very much a different piece of software, with a different design, and a different set of issues. Splitting up the core SPIRE offerings, each into their own sub-chart, now creates more effort in trying to keep the sub-charts coordinated, where one item somewhere (controller manager, for example) will require another item be present in a different root chart. That robs Helm of it's primary duty, to ensure a installation of the entire application, across all microservices, by putting the burden of a working implementation on the deplorer to correctly pick all the needed sub-charts.
This has already hurt adoption, such that all of the customers I maintain have decided to continue the efforts of maintaining simpler chart setups. They want the one-application / one-chart distinction, and they are defining one application as a whole SPIRE cluster, not an isolated Agent or an isolated Server.
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
de8bb38
to
817c62f
Compare
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>
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
No description provided.