KEP-9 and 10 Updates which includes all possible user facing YAML possibilities #847

kensipe · 2019-09-19T21:15:09Z

This PR now is consist with the Go Struct changes in #863

It removes the controversial schema notation which will be add back in later. Please look with a new set of eyes.

DETAILS FROM ORIGINAL PR BELOW
What this PR does / why we need it:

Expose full details of what is available to an operator developer. Previously a user was required to look at examples and source code to have full knowledge of what was possible in operator, params, and index yaml files
Provide a basis for tech discussion around the richness of the model. To find gaps in expression and requirements. To unify the mental model around this and provide a firm target for v1.
To clean up the keps, as the TOCs were incorrect. There is still more clean necessary but it is outside the scope of this PR.
Following this PR we should:

create a schema or otherwise provide validation for these constraints
add these details in more of a document format to our docs

What I would like to see as part of the review process:

Is the model rich enough (ignoring feature add ons like extensions which are coming)
Is something missing? could something be more or less rich?
all fields are required unless noted with required-False... Is something required that shouldn't be or vice versa
do the desc make sense... can / should they be elaborated?

kensipe · 2019-09-19T21:23:47Z

also... dependencies is defined in Operator. I did not include it.

alenkacz

I have just a few quite small comments around the descriptions and fields...

I think we should definitely note what language we use for that validation scheme, it's a bit confusing for the reader then...

alenkacz · 2019-09-20T08:19:39Z

keps/0009-operator-toolkit.md

-version: "5.7"
-kudoVersion: ">= 0.2.0"
-kubeVersion: ">= 1.14"
+name: str(max=253, lowercase alphanumeric characters, -, and .)


is this some validation/spec language? if so, should we probably not what we're using here? It kind of looks like a golang to me but the notation says yaml so I am a bit confused...

Same question here: what kind of Yaml spec language is this? Is there a parser for it?

although if this IS some sort of spec language that is already known, I would totally use this in params.yaml so we could get nested values :o

it is vaguely close to https://github.com/23andMe/Yamale
the point was to have condensed yet understandable constraints.
even in the psuedo sense... it seems understandable

not to replace a real schema at some pt of course

I'm torn on this one: if we're talking about defining a real schema then I'd go with OpenAPI and keep it outside of the KEP. An even better approach would be to generate these to keep them in sync with the rest of the codebase but I don't have a solution right now, so we might need to research our options here first.

The proposed pseudo schema is neither generated nor is it parsed and raises questions of why it is in this form. In this case, it can simply be examples with YAML comments describing individual fields.

alenkacz · 2019-09-20T08:21:46Z

keps/0009-operator-toolkit.md

+name: str(max=253, lowercase alphanumeric characters, -, and .)
+description: str(required=False)
+version: semver(desc='version of operator')
+appVersion: str(desc='component controlled version', required=False)


I think we could do better with desc here. I am not sure what is component and how is it controlling a version. I feel like it's hard to explain the difference between these two versions in one sentence though :/

Agree. This is the first time a "component" is mentioned and the distinction between appVersion and operatorVersion is not clear.

I think the verbosity of the difference will need to be elaborated in docs

alenkacz · 2019-09-20T08:22:30Z

keps/0009-operator-toolkit.md

+description: str(required=False)
+version: semver(desc='version of operator')
+appVersion: str(desc='component controlled version', required=False)
+kudoVersion: semver(desc='minVersion of KUDO')


what about - 'minimal required version (semver) of KUDO required for this operator to work'

Not part of this PR, but now, that I'm looking at it, we might want to make it a string to support Pipfile versioning style e.g. "<2.7.0,>=2.1" or ==3.4` in the future.

alenkacz · 2019-09-20T08:22:42Z

keps/0009-operator-toolkit.md

+version: semver(desc='version of operator')
+appVersion: str(desc='component controlled version', required=False)
+kudoVersion: semver(desc='minVersion of KUDO')
+kubernetesVersion: semver(desc='minVersion of Kubernetes')


what about - 'minimal required version (semver) of Kubernetes required for this operator to work'

alenkacz · 2019-09-20T08:24:16Z

keps/0009-operator-toolkit.md

+      - str(desc='name of resource')
+plans: map[string]Plan
+  str(max 253, desc='name of plan'):
+    strategy: ordering(*serial|parallel, desc='determines how to order phases', required=False)


should we also document what is the default somewhere since it's not required?

Currently this is required in code... this was a suggestion to make it not required and providing default information is super useful... that was the intent of * in *serial but that could be better.

alenkacz · 2019-09-20T08:27:53Z

keps/0010-package-manager.md

+  - name:  str(max=253)
+    version: semver()
+    appVersion: str(desc='component controlled version', required=False)
+    home: url(desc='home page, git repo', required=False)


in operator we call this url, I think home is a bit too generic here. I was immediately thinking about home directory :D I would personally omit this value from index altogether, it's inside operator, we can always add it here if the metadata is needed. It's hard to remove, but it's easy to add :)

this is already part of our code... are you suggesting that we remove it from code? or just not expose it?

I'm not sure who was initially responsible for the original code and yaml... this yaml has urls, home and source. There is a url in operator we should consolidate on the same nomenclature.

I'll look into that. Also agree on easier to add in later.

alenkacz · 2019-09-20T08:28:42Z

keps/0010-package-manager.md

+    version: semver()
+    appVersion: str(desc='component controlled version', required=False)
+    home: url(desc='home page, git repo', required=False)
+    sources: array(str, required=False)


wow, why is sources a list? Do we expect operator source to live in multiple repos? I also don't feel like this should be in index right now though

this is already part of our code... part of the intent of this PR was to surface all of what is current. Is your proposal to remove from code? or to reduce to 1? or to not expose?

alenkacz · 2019-09-20T08:29:29Z

keps/0010-package-manager.md

+    maintainers: array(Maintainer, required=False)
+    - name: str()
+      email: email()
+    deprecated: bool(desc='reflects if operator is deprecated')    


maybe the description should rather say what deprecated means for us? Can you still install? I honestly don't know how exactly we interpret this

i'm wondering if should do this by GVK. don't deprecate at the field level, deprecate at the GVK. that way, we just support multiple GVKs, deprecate a given GVK in a given release, and remove it after that.

I'm not sure I follow completely... that said is that a vote to remove deprecated?

Yes. My vote is to deprecate the entire "version" of our operator.yaml spec by adding an apiVersion tag to it at some point, rather than deprecate individual fields.

zen-dog

Aside from a few nits, my biggest question is about Yaml schema itself.

zen-dog · 2019-09-20T10:35:31Z

keps/0009-operator-toolkit.md

-version: "5.7"
-kudoVersion: ">= 0.2.0"
-kubeVersion: ">= 1.14"
+name: str(max=253, lowercase alphanumeric characters, -, and .)


Same question here: what kind of Yaml spec language is this? Is there a parser for it?

zen-dog · 2019-09-20T10:40:10Z

keps/0009-operator-toolkit.md

+name: str(max=253, lowercase alphanumeric characters, -, and .)
+description: str(required=False)
+version: semver(desc='version of operator')
+appVersion: str(desc='component controlled version', required=False)


Agree. This is the first time a "component" is mentioned and the distinction between appVersion and operatorVersion is not clear.

zen-dog · 2019-09-20T10:57:07Z

keps/0009-operator-toolkit.md

+description: str(required=False)
+version: semver(desc='version of operator')
+appVersion: str(desc='component controlled version', required=False)
+kudoVersion: semver(desc='minVersion of KUDO')


Not part of this PR, but now, that I'm looking at it, we might want to make it a string to support Pipfile versioning style e.g. "<2.7.0,>=2.1" or ==3.4` in the future.

keps/0009-operator-toolkit.md

gerred · 2019-09-20T11:18:10Z

The format questions aside..

model is definitely rich enough, though I do wonder if we should go more with the CRD's OpenAPIv3 format if this isn't already a spec of some sort, just to take advantage of work there and make this parseable.
this inspires a question of if params.yaml should actually be in either this spec format or OpenAPIv3...what if users were writing this? we could get nested parameters!
idk, need to think about that one, because different projects go both ways. I have no inclination at the moment.
i think in this format they should not be elaborated on, that's for docs pages, the short version is good here.

zen-dog · 2019-09-20T11:41:47Z

...and since we're defining a formal schema, wdyt about putting it into /schema folder and reference it in KEPs?

kensipe · 2019-10-02T21:26:08Z

There is still a need to clean up the keps... I will remove the controversial elements and make this mergeable.

kensipe · 2019-10-02T21:44:45Z

This PR now is consist with the Go Struct changes in #863

It removes the controversial schema notation which will be add back in later. Please look with a new set of eyes.

alenkacz

LGTM have just two small nits

keps/0009-operator-toolkit.md

keps/0010-package-manager.md

gerred

made one suggestion on top of alena's comments, I went ahead and generated a sha256 with some random thing. :D

zen-dog

I agree with the existing comments. Otherwise lgtm 🚢

alenkacz · 2019-12-12T13:55:34Z

@kensipe what do we do with this PR?

alenkacz · 2020-01-31T09:16:14Z

@kensipe ping

updating params array updates based on pr feedback Update to keps without schema details which will be updated at a later date updates based on pr feedback Signed-off-by: Ken Sipe <kensipe@gmail.com>

kensipe · 2020-03-06T21:32:18Z

thanks @alenkacz this hasn't been top of mind... cleaned it up and ready to go. thanks for the feedback

kensipe · 2020-03-06T23:10:15Z

Plenty of approvals... updates made for all feedback... rebased and ready to merge

Signed-off-by: Ken Sipe <kensipe@gmail.com> Signed-off-by: Thomas Runyon <runyontr@gmail.com>

kensipe requested a review from gerred as a code owner September 19, 2019 21:15

kensipe requested review from alenkacz, nfnt and zen-dog September 19, 2019 21:24

alenkacz reviewed Sep 20, 2019

View reviewed changes

zen-dog reviewed Sep 20, 2019

View reviewed changes

kensipe changed the title ~~User Facing YAML Design / Schema~~ Tech Conversation: User Facing YAML Design Sep 23, 2019

kensipe mentioned this pull request Sep 27, 2019

Removal of Struct Elements Based on Agreements for YAML Changes #863

Merged

kensipe force-pushed the ken/user-facing-yaml branch from 40d928b to a272856 Compare October 2, 2019 21:42

kensipe changed the title ~~Tech Conversation: User Facing YAML Design~~ KEP-9 and 10 Updates which includes all possible user facing YAML possibilities Oct 2, 2019

alenkacz approved these changes Oct 3, 2019

View reviewed changes

keps/0009-operator-toolkit.md Outdated Show resolved Hide resolved

keps/0010-package-manager.md Outdated Show resolved Hide resolved

gerred reviewed Oct 3, 2019

View reviewed changes

keps/0010-package-manager.md Outdated Show resolved Hide resolved

gerred approved these changes Oct 3, 2019

View reviewed changes

zen-dog approved these changes Oct 3, 2019

View reviewed changes

ANeumann82 added the kind/kep Kudo Enhancement label Dec 10, 2019

kensipe force-pushed the ken/user-facing-yaml branch from a272856 to 24f2f14 Compare March 6, 2020 21:18

added schema like details, cleaned up examples and TOC

b81fa41

updating params array updates based on pr feedback Update to keps without schema details which will be updated at a later date updates based on pr feedback Signed-off-by: Ken Sipe <kensipe@gmail.com>

kensipe force-pushed the ken/user-facing-yaml branch from 74a9264 to b81fa41 Compare March 6, 2020 21:25

kensipe merged commit 484f621 into master Mar 6, 2020

kensipe deleted the ken/user-facing-yaml branch March 6, 2020 23:11

runyontr pushed a commit that referenced this pull request Mar 11, 2020

KEP-0009 and KEP-0010 Updates with Examples and TOC (#847)

4f837a7

Signed-off-by: Ken Sipe <kensipe@gmail.com> Signed-off-by: Thomas Runyon <runyontr@gmail.com>

KEP-9 and 10 Updates which includes all possible user facing YAML possibilities #847

KEP-9 and 10 Updates which includes all possible user facing YAML possibilities #847

Conversation

kensipe commented Sep 19, 2019 • edited Loading

kensipe commented Sep 19, 2019

alenkacz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zen-dog Sep 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gerred Sep 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zen-dog left a comment

Choose a reason for hiding this comment

zen-dog Sep 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gerred commented Sep 20, 2019

zen-dog commented Sep 20, 2019

kensipe commented Oct 2, 2019

kensipe commented Oct 2, 2019

alenkacz left a comment

Choose a reason for hiding this comment

gerred left a comment

Choose a reason for hiding this comment

zen-dog left a comment

Choose a reason for hiding this comment

alenkacz commented Dec 12, 2019

alenkacz commented Jan 31, 2020

kensipe commented Mar 6, 2020

kensipe commented Mar 6, 2020

kensipe commented Sep 19, 2019 •

edited

Loading

zen-dog Sep 20, 2019 •

edited

Loading

gerred Sep 20, 2019 •

edited

Loading

zen-dog Sep 20, 2019 •

edited

Loading