Update global section to not have "additionalProperties: false" #42

inteon · 2024-10-04T12:28:37Z

⚠️ The code changes in this PR were outdated, this PR contains the right code changes instead: #43

Removes "additionalProperties: false" from the global section in values.yaml and all its nested sections.
Global is a special Helm values.yaml section that is shared across all charts, for that reason, we cannot be strict and must allow unknown fields.
See https://helm.sh/docs/chart_template_guide/subcharts_and_globals/#global-chart-values for more info about global values.

Fixes cert-manager/cert-manager#7329

diff --git a/schema_old.json b/schema.json
index a6d9901..1616822 100644
--- a/schema_old.json
+++ b/schema.json
@@ -575,152 +575,148 @@
       "default": ""
     },
     "helm-values.global": {
       "description": "Global values shared across all (sub)charts",
       "type": "object",
       "properties": {
         "commonLabels": {
           "$ref": "#/$defs/helm-values.global.commonLabels"
         },
         "imagePullSecrets": {
           "$ref": "#/$defs/helm-values.global.imagePullSecrets"
         },
         "leaderElection": {
           "$ref": "#/$defs/helm-values.global.leaderElection"
         },
         "logLevel": {
           "$ref": "#/$defs/helm-values.global.logLevel"
         },
         "podSecurityPolicy": {
           "$ref": "#/$defs/helm-values.global.podSecurityPolicy"
         },
         "priorityClassName": {
           "$ref": "#/$defs/helm-values.global.priorityClassName"
         },
         "rbac": {
           "$ref": "#/$defs/helm-values.global.rbac"
         },
         "revisionHistoryLimit": {
           "$ref": "#/$defs/helm-values.global.revisionHistoryLimit"
         }
-      },
-      "additionalProperties": false
+      }
     },
     "helm-values.global.commonLabels": {
       "description": "Labels to apply to all resources\nPlease note that this does not add labels to the resources created dynamically by the controllers. For these resources, you have to add the labels in the template in the cert-manager custom resource: eg. podTemplate/ ingressTemplate in ACMEChallengeSolverHTTP01Ingress\n   ref: https://cert-manager.io/docs/reference/api-docs/#acme.cert-manager.io/v1.ACMEChallengeSolverHTTP01Ingress\neg. secretTemplate in CertificateSpec\n   ref: https://cert-manager.io/docs/reference/api-docs/#cert-manager.io/v1.CertificateSpec",
       "type": "object",
       "default": {}
     },
     "helm-values.global.imagePullSecrets": {
       "description": "Reference to one or more secrets to be used when pulling images\nref: https://kubernetes.io/docs/tasks/configure-pod-container/pull-image-private-registry/\n\nFor example:\nimagePullSecrets:\n  - name: \"image-pull-secret\"",
       "type": "array",
       "default": [],
       "items": {}
     },
     "helm-values.global.leaderElection": {
       "type": "object",
       "properties": {
         "leaseDuration": {
           "$ref": "#/$defs/helm-values.global.leaderElection.leaseDuration"
         },
         "namespace": {
           "$ref": "#/$defs/helm-values.global.leaderElection.namespace"
         },
         "renewDeadline": {
           "$ref": "#/$defs/helm-values.global.leaderElection.renewDeadline"
         },
         "retryPeriod": {
           "$ref": "#/$defs/helm-values.global.leaderElection.retryPeriod"
         }
-      },
-      "additionalProperties": false
+      }
     },
     "helm-values.global.leaderElection.leaseDuration": {
       "description": "The duration that non-leader candidates will wait after observing a leadership renewal until attempting to acquire leadership of a led but unrenewed leader slot. This is effectively the maximum duration that a leader can be stopped before it is replaced by another candidate.",
       "type": "string"
     },
     "helm-values.global.leaderElection.namespace": {
       "description": "Override the namespace used for the leader election lease",
       "type": "string",
       "default": "kube-system"
     },
     "helm-values.global.leaderElection.renewDeadline": {
       "description": "The interval between attempts by the acting master to renew a leadership slot before it stops leading. This must be less than or equal to the lease duration.",
       "type": "string"
     },
     "helm-values.global.leaderElection.retryPeriod": {
       "description": "The duration the clients should wait between attempting acquisition and renewal of a leadership.",
       "type": "string"
     },
     "helm-values.global.logLevel": {
       "description": "Set the verbosity of cert-manager. Range of 0 - 6 with 6 being the most verbose.",
       "type": "number",
       "default": 2
     },
     "helm-values.global.podSecurityPolicy": {
       "type": "object",
       "properties": {
         "enabled": {
           "$ref": "#/$defs/helm-values.global.podSecurityPolicy.enabled"
         },
         "useAppArmor": {
           "$ref": "#/$defs/helm-values.global.podSecurityPolicy.useAppArmor"
         }
-      },
-      "additionalProperties": false
+      }
     },
     "helm-values.global.podSecurityPolicy.enabled": {
       "description": "Create PodSecurityPolicy for cert-manager\n\nNOTE: PodSecurityPolicy was deprecated in Kubernetes 1.21 and removed in 1.25",
       "type": "boolean",
       "default": false
     },
     "helm-values.global.podSecurityPolicy.useAppArmor": {
       "description": "Configure the PodSecurityPolicy to use AppArmor",
       "type": "boolean",
       "default": true
     },
     "helm-values.global.priorityClassName": {
       "description": "Optional priority class to be used for the cert-manager pods",
       "type": "string",
       "default": ""
     },
     "helm-values.global.rbac": {
       "type": "object",
       "properties": {
         "aggregateClusterRoles": {
           "$ref": "#/$defs/helm-values.global.rbac.aggregateClusterRoles"
         },
         "create": {
           "$ref": "#/$defs/helm-values.global.rbac.create"
         }
-      },
-      "additionalProperties": false
+      }
     },
     "helm-values.global.rbac.aggregateClusterRoles": {
       "description": "Aggregate ClusterRoles to Kubernetes default user-facing roles. Ref: https://kubernetes.io/docs/reference/access-authn-authz/rbac/#user-facing-roles",
       "type": "boolean",
       "default": true
     },
     "helm-values.global.rbac.create": {
       "description": "Create required ClusterRoles and ClusterRoleBindings for cert-manager",
       "type": "boolean",
       "default": true
     },
     "helm-values.global.revisionHistoryLimit": {
       "description": "The number of old ReplicaSets to retain to allow rollback (If not set, default Kubernetes value is set to 10)",
       "type": "number"
     },
     "helm-values.http_proxy": {
       "description": "Configures the HTTP_PROXY environment variable for where a HTTP proxy is required",
       "type": "string"
     },
     "helm-values.https_proxy": {
       "description": "Configures the HTTPS_PROXY environment variable for where a HTTP proxy is required",
       "type": "string"
     },
     "helm-values.image": {
       "type": "object",
       "properties": {
         "digest": {
           "$ref": "#/$defs/helm-values.image.digest"
         },
         "pullPolicy": {

Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>

maelvls

I've reviewed the original problem and agree with the solution. The code looks OK. I haven't tried manually testing helm-tool for generating a schema though.

/lgtm
/approve

cert-manager-prow · 2024-10-07T12:46:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: maelvls

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [maelvls]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

maelvls · 2024-10-07T12:50:26Z

schema/render.go

@@ -50,9 +50,12 @@ func (t *treeLevel) Type() parser.Type {
 	return parser.TypeUnknown
 }

-func (t *treeLevel) add(path paths.Path, property parser.Property) error {
+func (t *treeLevel) add(path paths.Path, property parser.Property, pruneBranch bool) error {


@inteon I'm always a little nervous seeing these booleans; it it always hard to know what true or false means, for example:

root.add(property.Path, property, false)

I'd suggest going with smth like

type Mode int const ( PruneBranch = Mode(iota) DontPruneBranch )

I don't know if this snippet is correct, but you get the idea. Func calls would be much more explicit:

root.add(property.Path, property, DontPruneBranch)

Sorry, this PR contained the wrong changes.
See #43 instead.

inteon · 2024-10-07T12:54:51Z

This PR has been replaced with #43, it contained the wrong changeset.

cert-manager-prow bot added dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 4, 2024

Update global section to not have "additionalProperties: false"

1fc8483

Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>

inteon force-pushed the global_set_additionalProperties_false branch from cda2871 to 1fc8483 Compare October 4, 2024 12:54

cert-manager-prow bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 4, 2024

maelvls approved these changes Oct 7, 2024

View reviewed changes

cert-manager-prow bot added the lgtm Indicates that a PR is ready to be merged. label Oct 7, 2024

cert-manager-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 7, 2024

cert-manager-prow bot merged commit 461a108 into main Oct 7, 2024
4 checks passed

maelvls reviewed Oct 7, 2024

View reviewed changes

inteon mentioned this pull request Oct 7, 2024

Update global section to not have "additionalProperties: false" #43

Merged

jkroepke mentioned this pull request Oct 7, 2024

helm: allow additionalProperties in values.schema.json cert-manager/cert-manager#7334

Closed

This was referenced Oct 7, 2024

[CI] Self-upgrade merging self-upgrade-master into master cert-manager/cert-manager#7346

Merged

[CI] Merge self-upgrade-release-1.16 into release-1.16 cert-manager/cert-manager#7348

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Update global section to not have "additionalProperties: false" #42

Update global section to not have "additionalProperties: false" #42

Uh oh!

inteon commented Oct 4, 2024 •

edited

Loading

Uh oh!

maelvls left a comment

Uh oh!

cert-manager-prow bot commented Oct 7, 2024

Uh oh!

Uh oh!

maelvls Oct 7, 2024 •

edited

Loading

Uh oh!

inteon Oct 7, 2024

Uh oh!

inteon commented Oct 7, 2024

Uh oh!

Uh oh!

Update global section to not have "additionalProperties: false" #42

Update global section to not have "additionalProperties: false" #42

Uh oh!

Conversation

inteon commented Oct 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maelvls left a comment

Choose a reason for hiding this comment

Uh oh!

cert-manager-prow bot commented Oct 7, 2024

Uh oh!

Uh oh!

maelvls Oct 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

inteon Oct 7, 2024

Choose a reason for hiding this comment

Uh oh!

inteon commented Oct 7, 2024

Uh oh!

Uh oh!

inteon commented Oct 4, 2024 •

edited

Loading

maelvls Oct 7, 2024 •

edited

Loading