Skip to content
This repository has been archived by the owner on Mar 22, 2024. It is now read-only.

Breakout charts #251

Draft
wants to merge 14 commits into
base: main
Choose a base branch
from
Draft

Breakout charts #251

wants to merge 14 commits into from

Conversation

kfox1111
Copy link
Contributor

@kfox1111 kfox1111 commented Apr 30, 2023

As per previous discussions, this pr breaks the child charts into standalone charts.

This was requested by @bramaq and others.

fixes: #235

This patch sets the spire chart tests to always run. This enables
changes in tests to be tested and sets a base for split out charts.

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>
…rk it will take.

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>
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>
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>
@kfox1111 kfox1111 changed the title Breakout charts testing Breakout charts. Just testing Apr 30, 2023
@kfox1111 kfox1111 changed the base branch from main to always-run-tests April 30, 2023 23:51
@edwbuck
Copy link
Contributor

edwbuck commented May 8, 2023

@kfox1111 @faisal-memon @marcofranssen

Please open an Issue to make this testing accessible from the developer environment without the use of our Merge Request system, and then rework the merge request system to chain its testing into the developer environment testing approach.

Consider this a high-priority request. We are attempting to reduce the number of open MRs, so we can regain velocity of accepting merge requests instead of velocity in context switching between them.

@kfox1111 kfox1111 changed the title Breakout charts. Just testing Breakout charts May 10, 2023
Base automatically changed from always-run-tests to main May 15, 2023 00:39
@faisal-memon
Copy link
Contributor

faisal-memon commented May 15, 2023

I am in favor of doing this. i think it will be a nice for users to have the option to deploy the components individually.

@faisal-memon faisal-memon added this to the 0.9.x milestone May 15, 2023
@marcofranssen
Copy link
Contributor

marcofranssen commented May 17, 2023

I think we are all in favor to do this, but we also discussed and agreed to postpone it till the major refactors and new features are added, it will complicate the release process which will give us additional load.

IMHO park this PR and pick it up at a later stage once the other big changes are in. Probably that is the reason @kfox1111 also left it as a draft PR just to prototype the feasibility at this stage.

What we need to do first is refactor the test structure to support multiple top level charts in CI. Also there that is easier if all the other changes like nesting etc are in to not get into to much merge conlficts and keeping branches in sync.

@kfox1111
Copy link
Contributor Author

I think we are all in favor to do this, but we also discussed and agreed to postpone it till the major refactors and new features are added, it will complicate the release process which will give us additional load.

Yeah. Mostly it was to see how much work it might take. It for sure is very disruptive to existing pr's. Most of that caused just by file renames.

After doing it as a test, almost all the changes are adding of things to the existing charts such as Chart.yaml metadata to make the linter happy, adding a missing global: {} line to each subcharts values etc. Also making _spire-lib.yaml a proper library chart. We could merge that stuff without really any disruption and do the final renames / test work in a different pr? Might let us get most of the hard work out of the way and just have a little bit to do during a flag day where we rename things to do the final break out.

IMHO park this PR and pick it up at a later stage once the other big changes are in.

Sounds like the plan for it is not until at least 0.9.x. So kind of parked already.

Probably that is the reason @kfox1111 also left it as a draft PR just to prototype the feasibility at this stage.

Yup. After doing it, the PR is feasible as is if the merge issues are fixed. But the timing is probably not right.

What we need to do first is refactor the test structure to support multiple top level charts in CI. Also there that is easier if all the other changes like nesting etc are in to not get into to much merge conlficts and keeping branches in sync.

Yeah. I think thats why its out at least to 0.9.x.

Currently, the tests target just charts/spire so it will work with other charts in the charts/ directory, they just wont be tested directly. But, if those charts are included in charts/spire as a dependency, which they all should be, which will cause them all to be tested that way. So maybe its not so bad. They all are still being tested, just not individually.

Copy link
Contributor

@edwbuck edwbuck left a comment

Choose a reason for hiding this comment

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

I've spent a few days thinking about this.

Helm Charts is a package manager to ensure that a collection of microservices, called an Application, is deployed in a consistent, supported way.

If we are to support helm charts for items considered less than a complete application, then we are playing with redefining Helm's core tenets, saying that independent components of an Application are "applications".

For this reason I would highly suggest that we not perform this kind of splitting, and if we do, that we do so in different top-level offerings, outside of spire/charts.

@kfox1111
Copy link
Contributor Author

Breaking up subcharts is commonly done with helm charts and IMO isn't against the spirit of helm. For example, I use this chart regularly:
https://github.com/prometheus-community/helm-charts/tree/main/charts/kube-prometheus-stack

You can get a very nice integrated with your k8s cluster chart by installing that. or you can install the individual pieces you need as standalone charts if the unified one doesn't work for you.

It has allowed the unified chart workers to focus on making the user experience the best for their particular need, while not excluding all of the advanced ways individuals may need to assemble the pieces themselves.

@faisal-memon
Copy link
Contributor

I agree with @kfox1111. Users can choose to deploy the whole stack in a single chart, as individual components, or a hybrid of both.

@faisal-memon faisal-memon modified the milestones: 0.9.0, 0.10.0 Jun 13, 2023
@faisal-memon
Copy link
Contributor

Discussed on sync meeting today. @kfox1111 to break down this PR into some smaller components to make it easier to review.

@marcofranssen marcofranssen modified the milestones: 0.10.0, 0.11.0 Jun 27, 2023
@marcofranssen marcofranssen modified the milestones: 0.11.0, 0.12.0 Jul 19, 2023
@faisal-memon faisal-memon modified the milestones: 0.12.0, 0.13.0 Aug 8, 2023
@faisal-memon faisal-memon modified the milestones: 0.13.0, 0.14.0 Sep 6, 2023
@faisal-memon faisal-memon modified the milestones: 0.14.0, 0.15.0 Sep 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

decouple the subcharts from the main chart
4 participants