Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add default policy server settings #70

Merged
merged 2 commits into from
Apr 7, 2022
Merged

Add default policy server settings #70

merged 2 commits into from
Apr 7, 2022

Conversation

ereslibre
Copy link
Member

@ereslibre ereslibre commented Mar 15, 2022

Fixes: #76

This adds a global values.yaml file that can be consumed by the subcharts on certain attributes that might be shared across them.

For installation, the current documentation can be followed, running:

$ helm install --wait -n kubewarden --create-namespace kubewarden-crds charts/kubewarden-crds -f values.yaml
$ helm install --wait -n kubewarden kubewarden-controller charts/kubewarden-controller -f values.yaml
$ helm install --wait -n kubewarden kubewarden-defaults charts/kubewarden-defaults -f values.yaml

You can read more about global values at https://helm.sh/docs/chart_template_guide/subcharts_and_globals/#global-chart-values

@ereslibre ereslibre requested a review from a team as a code owner March 15, 2022 11:06
Copy link
Contributor

@raulcabello raulcabello left a comment

Choose a reason for hiding this comment

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

LGTM, but this will change once we have a separate chart for the default PolicyServer
I would love to find a way of installing the 3 charts, and use the same values file. That way we could use the same default PolicyServer name. I'm not sure if that is possible

@ereslibre ereslibre marked this pull request as draft March 16, 2022 14:10
@ereslibre
Copy link
Member Author

Converting to draft until prerequisites are merged and figured out.

@ereslibre
Copy link
Member Author

kubewarden/kubewarden-controller#175 was merged too and unblocked this.

I'm going to rebase this PR on top of main.

When a policy server is not provided, the controller will default the
policy server if required.
@ereslibre ereslibre marked this pull request as ready for review April 5, 2022 08:54
@ereslibre ereslibre requested review from viccuad, jvanz, raulcabello and a team April 5, 2022 08:54
Copy link
Contributor

@raulcabello raulcabello left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Copy link
Member

@viccuad viccuad left a comment

Choose a reason for hiding this comment

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

It looks ok to me, but with this we have 3 values files (1 per chart), plus the global values. I would prefer to merge them all into global values already, to only need one yaml as UX. But we can do that in a follow-up PR that closes #76.

@ereslibre
Copy link
Member Author

ereslibre commented Apr 6, 2022

@viccuad We can discuss about it. I am in principle more in favour for each chart to encapsulate the values that make sense to that specific chart, and on the globals section, put values that are to be shared (and need to be in sync) across more than one helm chart/subchart.

This discussion goes on at #76

@ereslibre ereslibre marked this pull request as draft April 6, 2022 11:22
@ereslibre
Copy link
Member Author

Converting to draft because if published the toplevel values.yaml is not going to be consumed by helm. Looking into how we could make this work.

@ereslibre ereslibre requested a review from flavio April 7, 2022 07:46
@ereslibre
Copy link
Member Author

Based on the discussions on issue #76 I have updated this PR to be a middle ground.

values.yaml are not overdesigned by adding multiple artificial toplevel keys. They look very much the same as they did. A new toplevel key common is added and it's duplicated across charts that require to access this common attributes.

This allows a user to reuse the same values.yaml across all charts, and for common settings to match without hassle.

If we agree to go on with that I think we can add some automation as @viccuad proposed for linting and ensuring that we don't get key overrides. This task could be done afterwards this merges.

Copy link
Member

@viccuad viccuad left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the effort on this topic.

Copy link
Contributor

@raulcabello raulcabello left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @ereslibre!

@ereslibre ereslibre marked this pull request as ready for review April 7, 2022 10:03
Copy link
Member

@jvanz jvanz left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -16,4 +16,4 @@ maintainers:
version: 0.4.2

# This is the version of kubewarden-controller container image to be used
appVersion: "v0.5.1"
Copy link
Member Author

Choose a reason for hiding this comment

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

This will be changed very soon, when we release the helm chart too.

@ereslibre ereslibre merged commit 644bb5e into kubewarden:main Apr 7, 2022
@ereslibre ereslibre deleted the up-to-date-observed-condition branch April 7, 2022 12:18
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.

Feature Request: Use the same values.yml for the 3 charts
5 participants