Closed aiyengar2 closed 2 years ago
package-ci finally passed! Seems like this PR is good to go without any further changes, other than those pointed out during a review.
Thanks for the comments @jiaqiluo! Ready for a re-review here.
I resolved all the comments where I just took your changes verbatim.
For anything else, I left comments and left unresolved above; feel free to resolve them if you agree or comment if you disagree.
All changes you suggested have been added to the last two commits I just pushed.
~need to address edge cases brought up in https://github.com/rancher/charts-build-scripts/pull/66#pullrequestreview-809043347 around tgz files within CRD charts being modified...~
Done
Resolves https://github.com/rancher/charts-build-scripts/issues/35, https://github.com/rancher/charts-build-scripts/issues/46, https://github.com/rancher/charts-build-scripts/issues/56, https://github.com/rancher/charts-build-scripts/issues/61, https://github.com/rancher/charts-build-scripts/issues/62, https://github.com/rancher/charts-build-scripts/issues/63
https://github.com/rancher/charts-build-scripts/issues/54 is technically resolved, but not done in the way prescribed in the issue. Since we are moving to a newer system-charts model, there's no definition of what "validating charts" are, so instead the new behavior is to force a failure on validation if a chart is not included in the
release.yaml
.Therefore, when rebasing charts, a developer will:
make validate
failrelease.yaml
; the hope is that a developer would also catch here that the older, unrebased, unreleased version of the chart still existsmake remove
command.https://github.com/rancher/charts-build-scripts/issues/33 is no longer a concern since we just flat-out do not allow comments in Chart.yaml anymore. It will be overwritten on a
make prepare
, since that's what would happen when we run ahelm package
anyways on the chart.Related Issue: https://github.com/rancher/charts-build-scripts/issues/60