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

redpanda: generate values.schema.json from go structs #1090

Merged
merged 1 commit into from
Mar 22, 2024

Conversation

chrisseto
Copy link
Contributor

Based on PR #1089

This commit introduces a set of go structs that are now used to generate
the values.schema.json files for the redpanda chart.

This is being done to:

  • Ensure that values.schema.json is always up to date.
  • Allow correct and ergonomic go code to be written in place of helm
    templates.
  • Provide a more tenable way of defining, updating, and shaping the
    input values of the redpanda chart.

Efforts were taken to minimize the diff between the original and now
generated jsonschema but there is still a non-trivial diff due to many
invalid entries within the original jsonschema.

Future commits will slowly remove the hacks and shims once the initial
generation hurdle has been cleared.

Rather than reviewing a standard diff. It is recommended to instead view
the diff output by:

dyff between -i --exclude-regexp '.*annotations|.*labels' (git show origin/main:charts/redpanda/values.schema.json | psub) ./charts/redpanda/values.schema.json

  • dyff will simplify the overall diff.
  • -i ignores any change in the ordering of keys or elements within an
    array as neither are relevant in the context of a jsonschema.
  • --exclude-regexp '.*annotations|.*labels' many label and annotation
    fields did not have typed keys. As the Kubernetes API server would
    validate that all values are strings, this is now enforced at the
    schema level.

For reviewers thereof, here is an explanation of the larger diffs:

properties.storage.properties.tiered.properties.credentialsSecretRef
was missing a properties key in the handwritten schema.

properties.statefulset.properties.sideCars.properties.configWatcher.controllers
was incorrectly nested within configWatcher and missing a
properties key. It's now been hoisted to the correct location within
sideCars and formatted correctly.

properties.logging used the key parameters instead of properties.

@chrisseto
Copy link
Contributor Author

Here is the output of the dyff command from the commit message:

     _        __  __
   _| |_   _ / _|/ _|  between /var/folders/jy/5fvcqh9937d82pv9py94t4jc0000gp/T//.psub.PiK8jKcwZa
 / _' | | | | |_| |_       and ./charts/redpanda/values.schema.json
| (_| | |_| |  _|  _|
 \__,_|\__, |_| |_|   returned 20 differences
        |___/

properties.auditLogging.properties.enabledEventTypes
  + one map entry added:
    items:
      type: string

properties.auditLogging.properties.excludedPrincipals
  + one map entry added:
    items:
      type: string

properties.auditLogging.properties.excludedTopics
  + one map entry added:
    items:
      type: string

properties.auth.properties.sasl.properties.users
  - one map entry removed:
    minItems: 0

properties.auth.properties.sasl.properties.users.items
  + one map entry added:
    type: object

properties.commonLabels
  + one map entry added:
    additionalProperties:
      type: string

properties.external.properties.addresses
  + one map entry added:
    items:
      type: string

properties.external.properties.sourceRanges
  + one map entry added:
    items:
      type: string

properties.logging
  - one map entry removed:
    parameters:
      logLevel:
        type: string
        pattern: ^(error|warn|info|debug|trace)$
      usageStats:
        type: object
        required:
        - enabled
        properties:
          enabled:
            type: boolean
  
  + one map entry added:
    properties:
      logLevel:
        type: string
        pattern: ^(error|warn|info|debug|trace)$
      usageStats:
        type: object
        required:
        - enabled
        properties:
          enabled:
            type: boolean
  

properties.nodeSelector
  + one map entry added:
    additionalProperties:
      type: string

properties.statefulset.properties.additionalRedpandaCmdFlags
  + one map entry added:
    items:
      type: string

properties.statefulset.properties.nodeSelector
  + one map entry added:
    additionalProperties:
      type: string

properties.statefulset.properties.sideCars.properties
  + one map entry added:
    controllers:
      type: object
      properties:
        resources: true
        securityContext: true
        enabled:
          type: boolean
        image:
          type: object
          required:
          - tag
          - repository
          properties:
            repository:
              type: string
              default: docker.redpanda.com/redpandadata/redpanda-operator
              pattern: ^[a-z0-9-_/.]+$
            tag:
              type: string
              default: Chart.appVersion
              pattern: "^v(0|[1-9]\d*)\.(0|[1-9]\d*)\.(0|[1-9]\d*)(?:-((?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*)(?:\.(?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*))*))?(?:\+([0-9a-zA-Z-]+(?:\.[0-9a-zA-Z-]+)*))?$|^$"
    
  

properties.statefulset.properties.sideCars.properties.configWatcher
  - one map entry removed:
    controllers:
      type: object
      properties:
        resources:
          type: object
        enabled:
          type: boolean
        healthProbeAddress:
          type: string
        metricsAddress:
          type: string
        run:
          type: array
        securityContext:
          type: object
        image:
          type: object
          description: "Values used to define the container image to be used for Redpanda"
          required:
          - repository
          - tag
          properties:
            repository:
              type: string
              default: docker.redpanda.com/redpandadata/redpanda-operator
              description: "container image repository"
              pattern: ^[a-z0-9-_/.]+$
            tag:
              type: string
              default: Chart.appVersion
              description: "The container image tag. Use the Redpanda release version. Must be a valid semver prefixed with a 'v'."
              pattern: "^v(0|[1-9]\d*)\.(0|[1-9]\d*)\.(0|[1-9]\d*)(?:-((?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*)(?:\.(?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*))*))?(?:\+([0-9a-zA-Z-]+(?:\.[0-9a-zA-Z-]+)*))?$|^$"
    
  

properties.statefulset.properties.tolerations
  + one map entry added:
    items: true

properties.statefulset.properties.topologySpreadConstraints.items
  + one map entry added:
    type: object

properties.storage.properties.tiered.properties.credentialsSecretRef.properties.accessKey
  - three map entries removed:     + one map entry added:
    name:                            properties:
      type: string                     name:
    configurationKey:                    type: string
      type: string                     configurationKey:
    key:                                 type: string
      type: string                     key:
                                         type: string

properties.storage.properties.tiered.properties.credentialsSecretRef.properties.secretKey
  - three map entries removed:     + one map entry added:
    name:                            properties:
      type: string                     name:
    configurationKey:                    type: string
      type: string                     configurationKey:
    key:                                 type: string
      type: string                     key:
                                         type: string

properties.tls.properties.certs.patternProperties.^[A-Za-z_][A-Za-z0-9_]*$.properties.issuerRef
  - one map entry removed:
    additionalProperties: false

properties.tolerations
  + one map entry added:
    items:
      type: object


Copy link
Contributor

@alejandroEsc alejandroEsc left a comment

Choose a reason for hiding this comment

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

A lot to review, two questions, but not blocking.

chart:redpanda:generate:
cmds:
- go run ./cmd/genpartial/main.go -out charts/redpanda/values_partial.gen.go -struct Values ./charts/redpanda
- go run ./cmd/genvalues/main.go > charts/redpanda/values.schema.json
Copy link
Contributor

Choose a reason for hiding this comment

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

going the reverse of what we had, interesting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How do you mean? Were we generating something from the json schema previously?

Copy link
Contributor

Choose a reason for hiding this comment

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

exactly. I took an existing tool to take schemas from us and create apis.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What was the output of that and was it ever automated anywhere? The values.schema.json file as it currently exists is missing a good chunk of fields and it's fairly malformed as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, we never got around to make it automated, ill send you the link so you can see the repo.

Copy link
Contributor

Choose a reason for hiding this comment

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

charts/redpanda/values_util.go Outdated Show resolved Hide resolved
charts/redpanda/values_util.go Outdated Show resolved Hide resolved
@chrisseto chrisseto force-pushed the chris/partial-struct-generator branch 2 times, most recently from 453f90a to dc6fdf7 Compare March 19, 2024 17:34
@chrisseto chrisseto force-pushed the chris/generate-schema branch 2 times, most recently from da5c06b to 5a48c5c Compare March 19, 2024 17:42
Base automatically changed from chris/partial-struct-generator to main March 19, 2024 20:38
@chrisseto
Copy link
Contributor Author

Looks like this is going to be blocked until #1100 get's merged :/

This commit introduces a set of go structs that are now used to generate
the values.schema.json files for the redpanda chart.

This is being done to:
- Ensure that values.schema.json is always up to date.
- Allow correct and ergonomic go code to be written in place of helm
  templates.
- Provide a more tenable way of defining, updating, and shaping the
  input values of the redpanda chart.

Efforts were taken to minimize the diff between the original and now
generated jsonschema but there is still a non-trivial diff due to many
invalid entries within the original jsonschema.

Future commits will slowly remove the hacks and shims once the initial
generation hurdle has been cleared.

Rather than reviewing a standard diff. It is recommended to instead view
the diff output by:

`dyff between -i  --exclude-regexp '.*annotations|.*labels' (git show origin/main:charts/redpanda/values.schema.json | psub) ./charts/redpanda/values.schema.json`

- `dyff` will simplify the overall diff.
- `-i` ignores any change in the ordering of keys or elements within an
  array as neither are relevant in the context of a jsonschema.
- `--exclude-regexp '.*annotations|.*labels'` many label and annotation
  fields did not have typed keys. As the Kubernetes API server would
  validate that all values are strings, this is now enforced at the
  schema level.

For reviewers thereof, here is an explanation of the larger diffs:

`properties.storage.properties.tiered.properties.credentialsSecretRef`
was missing a `properties` key in the handwritten schema.

`properties.statefulset.properties.sideCars.properties.configWatcher.controllers`
was incorrectly nested within `configWatcher` _and_ missing a
`properties` key. It's now been hoisted to the correct location within
sideCars and formatted correctly.

`properties.logging` used the key `parameters` instead of `properties`.
@alejandroEsc alejandroEsc merged commit 3dc57a8 into main Mar 22, 2024
28 of 29 checks passed
@alejandroEsc alejandroEsc deleted the chris/generate-schema branch March 22, 2024 14:01
This pull request was closed.
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.

3 participants