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

KEP-35 Parameters extensions and json schema export #1698

Merged
merged 9 commits into from
Oct 2, 2020

Conversation

ANeumann82
Copy link
Member

What this PR does / why we need it:

This KEP describes:

  • Additional Parameter types (boolean and number)
  • Additional Attributes for UI generation
  • JSON-Schema export from the parameter list

This can be a first step to KEP-33 (Structured Parameters)

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>
Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>
Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>
Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>
Copy link
Contributor

@alenkacz alenkacz left a comment

Choose a reason for hiding this comment

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

Did a first pass, will come back to this tomorrow :)

keps/0035-json-schema-export.md Outdated Show resolved Hide resolved
All of the additional attributes are fully optional.

`group`
An attribute of type "string". This attribute describes a top-level group used for this parameter.
Copy link
Contributor

Choose a reason for hiding this comment

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

so this is just one level group?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes-ish. It would probably be easy enough to allow multiple levels by separating the groups by / or ., but for now only one level is supported. Although it might be a good idea to reserve a separation character already.
Any preferences?


### JSON-Schema export

Extend the `kudo package list parameters` with an option `--output json-schema` to describe the parameters as JSON-Schema.
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this option

Copy link
Member Author

Choose a reason for hiding this comment

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

I wonder how we can output a JSON-schema in YAML with this way though... --output yaml-schema is not really correct, --output json-schema-as-yaml might work, but is quite long...

And it might be nice to have the output as yaml, as the KEP-33 will support a JSON-Schema defined in a yaml file, so it would help with the conversion, and yaml is more common in the k8s world. But we can also add that later if required.

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>
Copy link
Contributor

@zen-dog zen-dog left a comment

Choose a reason for hiding this comment

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

Aside from the fact that we're dragging a lot of json-schema concerns into the current params.yaml this LGTM.

keps/0035-json-schema-export.md Outdated Show resolved Hide resolved
keps/0035-json-schema-export.md Outdated Show resolved Hide resolved
keps/0035-json-schema-export.md Outdated Show resolved Hide resolved

Extend the `kudo package list parameters` with an option `--output json-schema` to describe the parameters as JSON-Schema.

Alternatively, add a new command `kudo package list json-schema`, which then could be used with `--output json` or `--output yaml` to export the JSON-Schema in both variants.
Copy link
Contributor

Choose a reason for hiding this comment

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

The kudo package list parameters sounds better but I like the possibility of having YAML/JSON as output format.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've adjusted the proposal, found a (in my mind) even better solution.

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>
Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>
Copy link
Member

@nfnt nfnt left a comment

Choose a reason for hiding this comment

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

LGTM!


To bring the parameters up to par with JSON-schema, we add these additional types:
- `boolean` A "true" or "false" value, from the JSON "true" or "false" productions
- `number` An arbitrary-precision, base-10 decimal number value, from the JSON "number" production
Copy link
Member

Choose a reason for hiding this comment

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

In Javascript and therefore in JSON, number is a double-precision float, should we use that definition here instead?

`kudo package list parameters --output json-schema`
Outputs the parameters as JSON-Schema

`kudo package list parameters --output json-schema-yaml`
Copy link
Contributor

@zen-dog zen-dog Oct 1, 2020

Choose a reason for hiding this comment

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

Hm, I'm not sure about this. --output is about the output format. If we say that YAML is one and JSON too, can we really say that json-schema is one too? And json-schema-yaml sounds strange to me (what is it now? JSON? YAML? Both?)

Copy link
Member Author

Choose a reason for hiding this comment

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

I've had another approach here: f9e3294#diff-78066f3086b3541869b714f9b97d9814R143

Where I used --output and --format, but @alenkacz convinced me that using only --output would be ok. We copied that parameter from kubectl, which also uses multiple variants ( i.e. -o wide, etc.)

As for --output json-schema-yaml I agree that it's a bit cumbersome, but better than --output json-schema-as-yaml and --output yaml-schema is really wrong in my eyes.

I feel this is the most pragmatic approach

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>
Copy link
Contributor

@alenkacz alenkacz left a comment

Choose a reason for hiding this comment

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

Looks great, thank you 👏

@ANeumann82 ANeumann82 merged commit 25586ee into main Oct 2, 2020
@ANeumann82 ANeumann82 deleted the an/kep-34-json-schema-export branch October 2, 2020 10:14
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.

None yet

4 participants