-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
Document pluralization of API fields #2838
Conversation
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. |
34a71cf
to
1ab234d
Compare
@@ -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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this 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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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..
contributors/devel/api_changes.md
Outdated
|
||
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 |
There was a problem hiding this comment.
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.
1ab234d
to
e26bb61
Compare
Most comments addressed, also added words about de-duping plural. |
ping |
/lgtm |
/approve |
[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 |
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.