The generated schema does no longer work in kubeapps

[antgamdia]

Context: we (Kubeapps team) discovered that the Apache v8.8.0 broke our CI process because of the form not being rendered (because of the changes in [1]). This makes us think about the short-term future of the forms feature in Kubeapps [2].

Kubeapps relies on the custom form property to be true to render the values. I don't think it is something we should continue doing, but instead, rendering every property by default unless explicitly hidden.

Let me show an example with the old and new schema excerpts:

Old custom one:

"certManager": {
  "type": "boolean",
  "form": true,
  "title": "Enable TLS annotations via cert-manager",
  "description": "Set this to true in order to add the corresponding annotations for cert-manager",
  "hidden": {
    "value": false,
    "path": "ingress/enabled"
  }
},

New OpenAPI-compliant one:

"certManager": {
      "type": "boolean",
      "default": false,
      "description": "Add the corresponding annotations for cert-manager integration"
  },

We highlight here:

• the title property does not exist anymore, but it would be really useful for us, since the form labels will be clearer for the users.
• the form property does not exist anymore
• the hidden property does not exist anymore

Also, according to the spec (we're using 3.0.x, as far as I can remember), This object MAY be extended with Specification Extensions., so here are my suggestions:

• Add a title property, even if nothing is passed, it defaults to the description or the object key.
• Consider allowing custom extensions in the readme-generator. If so:
  • What about a x-kubeapps-form: false for hiding the forms?
  • What about a x-kubeapps-hidden: {} object for the same purpose as hidden is now for?

What do you think?

[1] https://github.com/bitnami/charts/commit/e3a8529be0fb7b3269e0105f2571d3b850891aa5#diff-ca31c7e60e6b0504ec10491ce8d590588c00763730b6ee3e18382f14f9cf688b

[2] https://github.com/kubeapps/kubeapps/issues/3526

[miguelaeh]

Hi @antgamdia , Thank you for the detailed explanation.

About the suggestions, we could add those new properties as modifiers, but, that would require adding them chart by chart in order to be consumed by Kubeapps. That from my point would be kind of confusing since we would be adding properties to the charts that for most of the charts users that do not use kubeapps would have no sense. Apart from that, modifying all the charts every time we or anyone needs a new property is not scalable at all.

With that in mind, my proposal is to create a new feature for the readme-generator to customize the metadata. To do that, we can allow passing a new file to the CLI like:

path.to.the.parameter.in.the.values: 
  x-kubeapps-form: false
  x-kubeapps-hidden: {}

Then, that object will be appended to the generated metadata, and either the Kubeapps team, or any other users can extend the generated metadata without the need of modifying the charts. Those teams can create their own "metadata extensions file".

Also, to avoid errors when the upstream charts' values are updated, we would add the necessary checks that the values paths in the extensions exist in the current chart, failing otherwise.

Would that proposal fit your needs?

[antgamdia]

Thanks for the quick response!

That from my point would be kind of confusing since we would be adding properties to the charts that for most of the charts users that do not use kubeapps would have no sense.

Yep, I also agree it shouldn't be coupled to the chart, that's why I suggested using extensions. It should be something totally optional and transparent for users who want to parse the schema with any other tool.

That said, I think we need to decide which is the best way to proceed from now on. In the past (and present, I'd say), the chart catalog did have information regarding how it should be presented in Kubeapps, so we just rely on it. If, from now on, we plan to alter this behavior (requiring an additional file appended to the metadata, for instance), it would require some extra effort on our side to adapt Kubeapps logic accordingly.

Then, that object will be appended to the generated metadata, and either the Kubeapps team, or any other users can extend the generated metadata without the need of modifying the charts. Those teams can create their own "metadata extensions file".

I get the big picture, but not 100% sure what you're suggesting: do you mean having the current annotated values.yaml plus an additional extensions.yaml file that, together, will yield the metadata file (with the extensions already applied) OR just having, as the output, the current metadata file plus the extensions.yaml so that this latter file is consumed by 3rd parties?

(from our side, it's not the same reading more props in an already downloaded file that start fetching a brand new file)

Another approach could be leveraging the conditionals in JSONSchema [1], and model the parameter's dependencies based on a combination of oneOf, allOf, dependentRequired, etc... however, the output schema (and the generation rules) would be way more complex.

The main issue, for us, is that new PRs and changes to the chart that also run this tool (like this commit) will lose information that is already present in the chart catalog (regardless this is a good idea or not). For us, it resulted in a CI failure and a delay in our release (since we depended on, precisely, the Apache chart to have a form :grimacing: ), but general users (and commercial ones) will get the forms they are used to see wiped out.

I think we have to arrange a meeting to discuss the future of the chart metadata and kubeapps visualization at some point.

(I mean, it is not this tool that is to blame, I'm super happy it finally exists and it's working so great! hehehe)

CCing @absoludity and @ppbaena

[1] https://json-schema.org/understanding-json-schema/reference/conditionals.html

[miguelaeh]

do you mean having the current annotated values.yaml plus an additional extensions.yaml file that, together, will yield the metadata file (with the extensions already applied)

Exactly.

The main issue, for us, is that new PRs and changes to the chart that also run this tool (like this commit) will lose information that is already present in the chart catalog (regardless this is a good idea or not)

I don't totally get how the information could be lost in the referenced PR. In principle, this tool will sync the metadata with the new chart changes so once that PR is merged the metadata should be re-generated and it will be synced with the new changes.

[absoludity]

I don't totally get how the information could be lost in the referenced PR. In principle, this tool will sync the metadata with the new chart changes so once that PR is merged the metadata should be re-generated and it will be synced with the new changes.

There was information already encoded in the values.schema.json which is not present or derivable from modifiers in the values.yaml. So when the readme-generator is used to generate the values.schema.json, information that we are currently using is lost. You can see this in the commit by looking at the diff for any of the files:

• bitnami/postgresql/values.schema.json, check the persistence property which defined form: true and a slider rendering,
• bitnami/apache/values.schema.json, check the certManager property which defined form: true and a dependency to hide if ingress is not enabled.

I don't remember how many charts in the bitnami catalog contained custom data in the values.schema.json but it was only a handlful. It would be great if we could avoid landing further changes that remove data like this until we have a way to cope, but understand if you can't delay.

The solution you've outlined @miguelaeh sounds great. If we could extract the existing data that was deleted from the values.schema.json of the apache and postgres charts in the above commit, into a separate file so that is passed as input to the readme generator such that the data gets encoded into the resulting values.schema.json metadata file, we can easily update to the new custom fields.

The question is whether we can delay the Kubeapps release until that is done. I guess not. It's already broken now since the current Apache chart no longer has the info.

[miguelaeh]

Hi @absoludity , I re-checked the PR, and I don't know why the values.schema.json was actually changed. It was a user contribution that overwrote the values.schema.json with the generated metadata, which is something we are not doing yet. I will proceed as the following in order to unblock your release:

1. Revert the changes to the values.schema.json in the PR.
2. Create the internal task to work on adding the feature we have talked about to be able to add extensions.
3. Once the feature is available we can move the values.schema.json extra information to that new extension file and delete them from the charts.

[absoludity]

Excellent, thanks @miguelaeh !

[miguelaeh]

The PR should unblock you for now. Setting as hold until we work on the new feature.

[absoludity]

Hi @miguelaeh . I'm just keen to ensure the proposal is clear before any work is started in the generator, so a few questions related to the proposal:

About the suggestions, we could add those new properties as modifiers, but, that would require adding them chart by chart in order to be consumed by Kubeapps.

By "modifiers", do you explicitly mean modifiers in the values.yaml that are currently used when generating the readme? If so, great, I think everyone agrees that the extra data should not be encoded in the values.yaml as modifiers.

That from my point would be kind of confusing since we would be adding properties to the charts that for most of the charts users that do not use kubeapps would have no sense.

Yep, any solution should only affect charts that choose to support a simpler UX for the values.

Apart from that, modifying all the charts every time we or anyone needs a new property is not scalable at all.

+1 .

With that in mind, my proposal is to create a new feature for the readme-generator to customize the metadata.

So I'm uncertain what you mean by the "metadata" here. Do you specifically mean the metadata within values.schema.json ? Or do you mean a separate file generated by the readme generator?

To do that, we can allow passing a new file to the CLI like:

path.to.the.parameter.in.the.values: 
  x-kubeapps-form: false
  x-kubeapps-hidden: {}

Great - a separate file referencing fields in the values/schema, with the specific data for the simpler UX for that chart sounds good, isolating that extra data. But I think Antonio and I are expecting that this file would be stored in the catalog with the chart (like it is currently), whereas I'm not sure whether that's what you're thinking? Note that the data can be potentially useful to any other UX that presents this info to users (even though it's only Kubeapps using it right now).

Then, that object will be appended to the generated metadata,

Can you clarify what this means? I'm not sure if you're proposing adding it to the generated values.schema.json or something else (and both options are possible and have pros and cons)

and either the Kubeapps team, or any other users can extend the generated metadata without the need of modifying the charts. Those teams can create their own "metadata extensions file".

So it's this sentence that leaves me thinking you mean that this info would be separate from the catalog, something managed by other teams. This worries me a bit because Kubeapps doesn't (currently) store any data about charts other than what's in the catalog. We could update to keeping a separate data store but that will be fraught with problems because the data is related to specific charts and chart versions of a chart repository. Users install whatever chart repository they want, without any correlation between our simple form data and the charts in a repo added by the user.

Also, to avoid errors when the upstream charts' values are updated, we would add the necessary checks that the values paths in the extensions exist in the current chart, failing otherwise.

Great, but this leaves me thinking that you do indeed mean for the data to be present in the catalog so that it can be verified when running the generator?

Would that proposal fit your needs?

Depending on the above, I think so. But there are a few other considerations worth looking at before we decide. I don't need you to specify a solution now, I just want to ensure that before we start on this, we write a short google doc outlining what is required, what the solution is etc., and we can have a meeting about it at that time.

FWIW, I wonder whether the file with the simple form data should be left separate from the values.schema.json, rather than integrating the metadata into it (perhaps just using the form you've specified above). This would have the following advantages:

• Assuming it's still included with the chart (and validated by some process, whether it's the generator or not), any UX (such as Kubeapps) can still use that data effectively in the same way that it could if it was embedded in the schema, but
• Users or other third parties (not Kubeapps) can choose to provide their own simple-form data for specific charts, even for charts that don't currently include that data (something that's been requested recently).

So longer term, it may be better for users of the data if it's not embedded in the values.schema.json. Let's discuss that option further at the time.

[miguelaeh]

Hi @absoludity ,

So I'm uncertain what you mean by the "metadata" here. Do you specifically mean the metadata within values.schema.json ? Or do you mean a separate file generated by the readme generator?

Currently, the readme-generator is generating a file (if you provide the -m flag) that stores the same metadata that it writes on the README table in an open API compliant JSON file. I think we could take advantage of that file to be used by Kubeapps instead of maintaining a values.schemas.json separately. That file could be stored wherever we decide, either in the charts repo or not, but the good thing is that it will be automatically generated, so it will be really easy to maintain. (I don't know about the implications to adapt kubeapps to consume that file instead)

Great - a separate file referencing fields in the values/schema, with the specific data for the simpler UX for that chart sounds good, isolating that extra data. But I think Antonio and I are expecting that this file would be stored in the catalog with the chart (like it is currently), whereas I'm not sure whether that's what you're thinking? Note that the data can be potentially useful to any other UX that presents this info to users (even though it's only Kubeapps using it right now).

This is something we can discuss, we could store that file with the charts, create a new folder just for that kind of file in the Helm Charts repository... There are several options here, we can talk about them to decide the best.

Great, but this leaves me thinking that you do indeed mean for the data to be present in the catalog so that it can be verified when running the generator?

Not exactly, the generator receives the file's paths as parameters so those files could be at any path in the system where the generator is executed.

FWIW, I wonder whether the file with the simple form data should be left separate from the values.schema.json, rather than integrating the metadata into it (perhaps just using the form you've specified above). This would have the following advantages:

Indeed, that was the proposal, let me explain it in another way just in case.

Currently, we have values.schema.json which is manually created. Nevertheless, the readme-generator is able to generate a JSON with the same metadata that we add to the README's tables. The propose is to get rid of the manual edited values.schema.json, instead, a similar file will be autogenerated. We just need to provide a way to the generator to know what it needs to add for the UI. Let me use an example. Let's take this values.yaml:

## @section Example Section

## @param some.ui.object An example to show the metadata
some:
  ui:
    object: "test"

It will generate the following table into the readme:

## Parameters

### Example Section

| Name             | Description                     | Value  |
| ---------------- | ------------------------------- | ------ |
| `some.ui.object` | An example to show the metadata | `test` |

Also, if we provide the -m flag, it will generate the following JSON:

{
    "title": "Chart Values",
    "type": "object",
    "properties": {
        "some": {
            "type": "object",
            "properties": {
                "ui": {
                    "type": "object",
                    "properties": {
                        "object": {
                            "type": "string",
                            "default": "test",
                            "description": "An example to show the metadata"
                        }
                    }
                }
            }
        }
    }
}

So, the idea is, once the generator has that JSON, we provide a new file like the following (no matter if it is on the catalog or not, we need to pass the path):

some.ui.object: 
  x-kubeapps-form: false
  x-kubeapps-hidden: {}

so, those extra properties are appended, resulting in:

{
    "title": "Chart Values",
    "type": "object",
    "properties": {
        "some": {
            "type": "object",
            "properties": {
                "ui": {
                    "type": "object",
                    "properties": {
                        "object": {
                            "type": "string",
                            "default": "test",
                            "description": "An example to show the metadata"
                            "x-kubeapps-form": false
                            "x-kubeapps-hidden": {}
                        }
                    }
                }
            }
        }
    }
}

Storing the values.schemas.json will not have sense anymore since they can be auto-generated at the very moment you need it, we will need to store just the file with the extra properties. If we want them on the catalog or not is something to decide. Another approach could be to store the generated metadata in the catalog while maintaining the files with extra properties outside the catalog, so it would be like replacing the content of the values.schema.json by something that is auto-generated and easy to read and maintain.

[fmulero]

I'll try to summarize this discussion adding some concerns recently appeared.

Background:

• Kubeapps uses a small subset of values currently defined in values.schema.json.
• values.schema.json files are are not actively maintained. Why? Maintain them manually is hard, not all values are covered intentionally and there non standard properties like form, render, hidden, etc. See vmware-tanzu/kubeapps#4917
• Users are requesting ways to enrich or modify the schema generated by Readme Generator for Helm See #27 or #34

Proposal:

• values.yaml file should be the source of truth, no additional files should be required to generate the schema.
• Kubeapps renderization properties should be defined as extensions
• Readme Generator for Helm should allow setting these extensions applying modifiers to the @param metadata. In order to keep simplicity maybe we have to revisit this feature and consider positional modifiers, example:

  ## @param replicas Number of replicas of the Apache deployment
  ## @deprecated
  ## @extension X-kubeapps-render slider
  replicas: 1

[justusbunsi]

@fmulero Wouldn't it be necessary for the extension metadata line to include the parameter name as well like it's done with @extra?

## @param replicas Number of replicas of the Apache deployment
## @deprecated
- ## @extension X-kubeapps-render slider
+ ## @extension replicas X-kubeapps-render slider 
replicas: 1

[fmulero]

Sorry for my late reponse.

To be honest I didn't have enough time to work on this topic, but your suggestion makes sense keeping in mind how we are using the @param metadata on top of some object definitions. For instance:

  ## Configure extra options for startup probe
  ## @param master.startupProbe.enabled Enable startupProbe
  ## @param master.startupProbe.initialDelaySeconds Initial delay seconds for startupProbe
  ## @param master.startupProbe.periodSeconds Period seconds for startupProbe
  ## @param master.startupProbe.timeoutSeconds Timeout seconds for startupProbe
  ## @param master.startupProbe.failureThreshold Failure threshold for startupProbe
  ## @param master.startupProbe.successThreshold Success threshold for startupProbe
  ##
  startupProbe:
    enabled: false
    initialDelaySeconds: 30
    periodSeconds: 10
    timeoutSeconds: 5
    failureThreshold: 6
    successThreshold: 1