Skip to content

Document pluralization of API fields #2838

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

Merged
merged 1 commit into from
Nov 2, 2018

Conversation

thockin
Copy link
Member

@thockin thockin commented Oct 22, 2018

Based on
https://docs.google.com/document/d/1Z8Vbo7RmV_Wvs4k8mluHQC2_Zs8cEwMJmHwWBf9BcaA/edit
and email discussions.

Resist the urge to clean up the whole doc - that will have to come
later.

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 22, 2018
@thockin
Copy link
Member Author

thockin commented Oct 22, 2018

This is almost verbatim from the doc, which was almost verbatim from the emails.

I tried to make the lead-up to this section cleaner. There are a lot of small updates and edits that this doc needs, but I did not do most of that here.

@thockin thockin force-pushed the how-to-pluralize-a-field branch from 34a71cf to 1ab234d Compare October 22, 2018 23:51
@@ -107,24 +109,34 @@ does not add a new required field)
* which fields are required and which are not
* mutable fields do not become immutable
* valid values do not become invalid
* explicitly invalid values do not become valid
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be confusing for a novice - how does valid values in previous bullet contrast to explicitly invalid, etc

Copy link
Member

Choose a reason for hiding this comment

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

s/explicitly/previously/ (if you meant something else, more explanation is needed)

Note that one might expect this and the prior line to have a symmetry over upgrade/downgrade, but they don't?

Copy link
Member Author

Choose a reason for hiding this comment

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

What I wanted to convey was the difference between "we expanded the regex to include dashes" from the case of open-ended enum fields, which is discussed elsewhere in the doc.

Suggestions?

current value

Older clients that only know the singular field will continue to succeed and
produce the same results as before the change. Newer clients can use your
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably worth noting that old clients break when trying to update the singular value of plural values are specified (because 0 has changed), since a lot people may not realize the implications

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually you have it below, disregard

Copy link
Member

@lavalamp lavalamp left a comment

Choose a reason for hiding this comment

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

mostly just nits.

@@ -107,24 +109,34 @@ does not add a new required field)
* which fields are required and which are not
* mutable fields do not become immutable
* valid values do not become invalid
* explicitly invalid values do not become valid
Copy link
Member

Choose a reason for hiding this comment

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

s/explicitly/previously/ (if you meant something else, more explanation is needed)

Note that one might expect this and the prior line to have a symmetry over upgrade/downgrade, but they don't?

continue to function as they did previously, even when your change is utilized.
5. Existing clients need not be aware of your change in order for them to
continue to function as they did previously, even when your change is in use.
6. It must be possible to rollback to a previous version of API server that
Copy link
Member

Choose a reason for hiding this comment

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

How many versions are required to be roll-backable? I think just 1, but we should say that?

5. Existing clients need not be aware of your change in order for them to
continue to function as they did previously, even when your change is in use.
6. It must be possible to rollback to a previous version of API server that
does not include your change and have no impact on API objects which do not use
Copy link
Member

Choose a reason for hiding this comment

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

...and what of objects that are using your change?

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'd add words, but there's not much to say - a feature got rolled back. There's always the possibility of weird behavior in those cases. What would you say here?

Copy link
Member

Choose a reason for hiding this comment

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

I think we owe it to people to give better guidance about what kinds of impact are acceptable. But we can fix that later, I think we don't actually have a great answer at the moment..


However, this is very challenging to do correctly. It often requires multiple
For some change, this can be challenging to do correctly. It may require multiple
Copy link
Member

Choose a reason for hiding this comment

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

s/change/changes/

Based on
https://docs.google.com/document/d/1Z8Vbo7RmV_Wvs4k8mluHQC2_Zs8cEwMJmHwWBf9BcaA/edit
and email discussions.

Resist the urge to clean up the whole doc - that will have to come
later.
@thockin thockin force-pushed the how-to-pluralize-a-field branch from 1ab234d to e26bb61 Compare October 23, 2018 19:14
@thockin
Copy link
Member Author

thockin commented Oct 23, 2018

Most comments addressed, also added words about de-duping plural.

@thockin
Copy link
Member Author

thockin commented Nov 2, 2018

ping

@lavalamp
Copy link
Member

lavalamp commented Nov 2, 2018

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 2, 2018
@cblecker
Copy link
Member

cblecker commented Nov 2, 2018

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cblecker, lavalamp

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:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 2, 2018
@k8s-ci-robot k8s-ci-robot merged commit dd065c7 into kubernetes:master Nov 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants