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

Update helm templates for compatibility with kustomize #1031

Merged
merged 7 commits into from
Feb 9, 2021
Merged

Update helm templates for compatibility with kustomize #1031

merged 7 commits into from
Feb 9, 2021

Conversation

smlx
Copy link
Contributor

@smlx smlx commented Dec 17, 2020

What this PR does / why we need it:

kustomize now does additional validation on YAML. This PR updates the helm chart templates for compatibility with kustomize.

Which issue(s) this PR fixes:
Closes #1022
Superseded / Closes: #980

Special notes for your reviewer:

The new kustomization patches are based on diffing the generated helm chart to avoid any unrelated changes.

Copy link
Contributor

@maxsmythe maxsmythe 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 tackling this!

@sozercan
Copy link
Member

@smlx there are some conflicts now, can you rebase when you get a chance?

@smlx
Copy link
Contributor Author

smlx commented Dec 22, 2020

OK, rebased.

@codecov-io
Copy link

codecov-io commented Dec 22, 2020

Codecov Report

Merging #1031 (e340018) into master (be2f5b4) will decrease coverage by 0.23%.
The diff coverage is 41.07%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1031      +/-   ##
==========================================
- Coverage   48.08%   47.84%   -0.24%     
==========================================
  Files          62       62              
  Lines        4274     4264      -10     
==========================================
- Hits         2055     2040      -15     
- Misses       1964     1966       +2     
- Partials      255      258       +3     
Flag Coverage Δ
unittests 47.84% <41.07%> (-0.24%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/audit/manager.go 0.00% <0.00%> (ø)
pkg/controller/constraint/constraint_controller.go 4.22% <0.00%> (-0.02%) ⬇️
pkg/controller/sync/sync_controller.go 0.00% <0.00%> (ø)
pkg/metrics/exporter.go 0.00% <0.00%> (ø)
pkg/readiness/ready_tracker.go 69.09% <ø> (-0.73%) ⬇️
pkg/util/pack.go 0.00% <0.00%> (ø)
pkg/watch/replay.go 71.08% <ø> (+0.96%) ⬆️
pkg/webhook/mutation.go 12.68% <0.00%> (ø)
pkg/webhook/policy.go 27.31% <0.00%> (ø)
pkg/target/target.go 66.03% <25.00%> (ø)
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9d424ff...e340018. Read the comment docs.

@sozercan
Copy link
Member

sozercan commented Dec 22, 2020

LGTM. I think you need to run make manifests and commit resulting changes too.

Update:
Looks like there is an issue deploying generated chart:
Error: YAML parse error on gatekeeper/templates/gatekeeper-audit-deployment.yaml: error converting YAML to JSON: yaml: line 38: mapping values are not allowed in this context

kustomize is now stricter on k8s object validation, so we need to
massage the templates and replacement strings a bit.

Signed-off-by: Scott Leggett <scott@sl.id.au>
Some values are hard-coded in existing base yaml. In helm these should
be controlled via values.yaml.

Signed-off-by: Scott Leggett <scott@sl.id.au>
Signed-off-by: Scott Leggett <scott@sl.id.au>
Signed-off-by: Scott Leggett <scott@sl.id.au>
Copy link
Member

@sozercan sozercan left a comment

Choose a reason for hiding this comment

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

LGTM

@sozercan
Copy link
Member

@smlx thanks for the PR! Looks like there are a few conflicts, please let us know if you get a chance to resolve them.

@smlx
Copy link
Contributor Author

smlx commented Jan 28, 2021

hi @sozercan sorry I've already rebased and fixed conflicts twice now since sending this PR and don't have the time/energy to do it again. Feel free to close or anyone is welcome to take over.

@sozercan
Copy link
Member

sozercan commented Feb 9, 2021

No worries at all, thanks for the PR again. I pushed to your branch to fix the conflicts.

@maxsmythe still lgty?

@maxsmythe
Copy link
Contributor

LGTM, I think there is one stray character in here though (search for :c)

@sozercan sozercan merged commit ccf9579 into open-policy-agent:master Feb 9, 2021
@smlx smlx deleted the update-helm-templates branch February 10, 2021 01:35
@smlx
Copy link
Contributor Author

smlx commented Feb 10, 2021

Thanks for getting this over the line, much appreciated! 🎉

jdolce pushed a commit to jdolce/gatekeeper that referenced this pull request Feb 12, 2021
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.

Helm chart generation incompatible with kustomize v3.8.8 (current latest)
5 participants