This repository has been archived by the owner on Mar 22, 2024. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 22
Move spire-lib macro's to their own library chart #346
Open
kfox1111
wants to merge
6
commits into
main
Choose a base branch
from
spire-lib-chart
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
817c62f
Move spire-lib macro's to their own library chart
kfox1111 ef4d9e9
Merge branch 'main' into spire-lib-chart
kfox1111 85c8013
Add dummy values
kfox1111 94330f1
Docs need to ignore spire-lib
kfox1111 18b046f
Docs need to ignore spire-lib
kfox1111 64e0556
Docs need to ignore spire-lib
kfox1111 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,6 +22,9 @@ maintainers: | |
| email: [email protected] | ||
| kubeVersion: ">=1.21.0-0" | ||
| dependencies: | ||
| - name: spire-lib | ||
| repository: file://./charts/spire-lib | ||
| version: 0.1.0 | ||
| - name: spire-server | ||
| condition: spire-server.enabled | ||
| repository: file://./charts/spire-server | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| # Patterns to ignore when building packages. | ||
| # This supports shell glob matching, relative path matching, and | ||
| # negation (prefixed with !). Only one pattern per line. | ||
| .DS_Store | ||
| # Common VCS dirs | ||
| .git/ | ||
| .gitignore | ||
| .bzr/ | ||
| .bzrignore | ||
| .hg/ | ||
| .hgignore | ||
| .svn/ | ||
| # Common backup files | ||
| *.swp | ||
| *.bak | ||
| *.tmp | ||
| *.orig | ||
| *~ | ||
| # Various IDEs | ||
| .project | ||
| .idea/ | ||
| *.tmproj | ||
| .vscode/ |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| apiVersion: v2 | ||
| name: spire-lib | ||
| description: A library of helper templates for spire | ||
| type: library | ||
| version: 0.1.0 | ||
| appVersion: "0.1.0" | ||
| home: https://github.com/spiffe/helm-charts/tree/main/charts/spire | ||
| sources: | ||
| - https://github.com/spiffe/helm-charts/tree/main/charts/spire | ||
| icon: https://spiffe.io/img/logos/spire/icon/color/spire-icon-color.png | ||
| maintainers: | ||
| - name: marcofranssen | ||
| email: [email protected] | ||
| url: https://marcofranssen.nl | ||
| - name: kfox1111 | ||
| email: [email protected] | ||
| - name: faisal-memon | ||
| email: [email protected] | ||
| - name: edwbuck | ||
| email: [email protected] |
File renamed without changes.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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.