Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
CR webhook conversion documentation #10986
CR webhook conversion documentation #10986
Changes from all commits
e4c11d6
5351b17
d5825b7
9d8d1ee
505e061
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
the first thing in this section should be a prominent note that the feature is alpha in 1.13, is not enabled by default, what enabling an alpha feature means, and how to enable it
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.
Can you provide a link to the Kubernetes
Versioning
doc?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.
@kbhawkey do you mean this: https://github.com/eBay/Kubernetes/blob/master/docs/design/versioning.md?
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.
Because this algorithm follows Kubernetes versioning, it may be helpful to link to the actual versioning doc, or incorporate that content to a new page in the k8s documentation.
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.
You could add a label to indicate that the feature is alpha, such as:
{{< feature-state for_k8s_version="v1.13" state="alpha" >}}
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.
@mbohlool, what do you think about adding the feature state button before the note below? It is not necessary, but makes it clear that webhook conversion is an alpha feature.
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.
nit: These conversions happen independently.
nit: The grouping of custom resources ...
What grouping? The order of custom resources listed in the conversion request?
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.
suggestion: "the conversion requests may contain multiple objects in order to minimize the calls to the webhook"
I think you can drop the sentence "The grouping of custom resources does not represent any relation between them."
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've revised this paragraph. please look at it again.
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.
LGTM, 👍, except for 404 on the link,
https://github.com/kubernetes/kubernetes/tree/v1.13.0/test/images/crd-conversion-webhook/main.go
Try:
https://github.com/kubernetes/kubernetes/blob/master/test/images/crd-conversion-webhook/main.go
@mbohlool, I was unable to get to this link from the preview page.
nit: There is a note on the page that should capitalize Kubernetes v1.13.
nit: Another note: When using ClientConfig.service ... where or what does this apply to, or is it unrelated to sample spec?
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.
the link will be valid when the 1.13.0 release is tagged. Linking to specific files in master is unstable.
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.
👍
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.
capitalized text kubernetes -> Kubernetes.
About ClientConfig.service, it is related to the sample as the sample has
ClientConfig.service
. The use of https is forced as to another note and here we are just stating what name should the server certificate be valid for, for this config to work.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.
this needs a bigger warning. It is highly discouraged to run a webhook this way in production.
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.
is client auth essential for conversion? for audit and some types of admission, I'd agree, but I'm not sure it is required in all cases for a conversion service
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.
You can remove the Note:. The template includes this text.
Should webhook be Webhook?
Also, is this PR missing the preview?
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.
Maybe add some per version schema to demonstrate capability of webhook conversion?
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 had something here, it got lost in my revisions. Nice suggestion.