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

fix: policy controller chart schema #904

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

falcorocks
Copy link
Contributor

@falcorocks falcorocks commented Feb 3, 2025

Description of the change

This PR removes 2 keys from the chart schema that were added by mistake in 142e34b (probably through the helm-schema, see https://sigstore.slack.com/archives/C03096V09F1/p1737627750048369):

  • additionalProperties
  • required

helm-schema gives for granted that all values in the values.yaml are required and should not allow additionalProperties, which is pretty inflexible. We should ask users to run helm-schema -k additionalProperties,required to avoid this from happening again. The PR also adds a new CI workflow to check that the schema has been generated with these flags. I've tested a failure on purpose here by messing with the schema and it worked. Notice that this test is limited to the policy controller only in this PR, if we are happy we can expand the test to cover all charts later.

Existing or Associated Issue(s)

Originally implemented in #894

Additional Information

Checklist

  • Chart version bumped in Chart.yaml according to semver. Where applicable, update and bump the versions in any associated umbrella chart
  • Variables are documented in the values.yaml and added to the README.md. The helm-docs utility can be used to generate the necessary content. Use helm-docs --dry-run to preview the content.
  • JSON Schema generated.
  • List tests pass for Chart using the Chart Testing tool and the ct lint command.

removes keys required and additonalProperties from the controller chart
schema that were added by mistake in 142e34b
see https://sigstore.slack.com/archives/C03096V09F1/p1737627750048369

Signed-off-by: falcorocks <[email protected]>
@falcorocks falcorocks marked this pull request as ready for review February 3, 2025 14:13
@falcorocks falcorocks changed the title fix: chart schema fix: policy controller chart schema Feb 6, 2025
@falcorocks falcorocks marked this pull request as draft February 6, 2025 09:55
@falcorocks falcorocks marked this pull request as ready for review February 6, 2025 09:55
Copy link
Member

@bobcallaway bobcallaway left a comment

Choose a reason for hiding this comment

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

seems fine to me, @hectorj2f any thoughts?

Comment on lines +14 to +15
uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
with:
persist-credentials: false

uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2

- name: setup go
uses: actions/setup-go@v5
Copy link
Member

Choose a reason for hiding this comment

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

please pin this at the latest release commit

Comment on lines +18 to +19
with:
go-version: '>=1.23.1'
Copy link
Member

Choose a reason for hiding this comment

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

i'd just remove this, since it will use the default go toolchain on the runner and that's sufficient for go install

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.

2 participants