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

📖 add doc for admission webhooks #776

Merged
merged 2 commits into from
Jul 11, 2019

Conversation

mengqiy
Copy link
Member

@mengqiy mengqiy commented Jun 11, 2019

There is a section about suggested development workflow with kind, so this fixes #400

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 11, 2019
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 11, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mengqiy

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 Jun 11, 2019
allErrs := c.validateCronJobSpec()
if len(c.ObjectMeta.Name) > apimachineryutilvalidation.DNS1035LabelMaxLength-11 {
// The cronjob controller appends a 11-character suffix to the cronjob (`-$TIMESTAMP`) when
// creating a job. The job name length limit is 63 characters.
Copy link
Contributor

Choose a reason for hiding this comment

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

say why -- the job name length is 63 character like all Kubernetes objects (which must fit in a DNS subdomain)

Copy link
Contributor

Choose a reason for hiding this comment

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

Add a note that we could validate this declaratively, but we keep it here as an example

Copy link
Member Author

Choose a reason for hiding this comment

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

Add a note that we could validate this declaratively

This is ObjectMeta.Name which is not owned by the user. Not sure if it's doable through validation schema. Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

it's explicitly doable, in fact. GenerateName and Name are the only things that are allowed to have validation, going forward. Unfortunately, we don't have a good way to accomplish it in KB, but it's doable in in Kubernetes.

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, that's what I meant.
Upstream k/k can add a marker for ObjectMeta.Name to have maximum 63 length. But as a downstream user, if I can't change the marker in upstream to accomplish my need which is 52 max length.

docs/book/src/cronjob-tutorial/webhook-implementation.md Outdated Show resolved Hide resolved
docs/book/src/cronjob-tutorial/webhook-overview.md Outdated Show resolved Hide resolved
@droot droot added this to Code Awaiting Review in 2.0.0 cross-project tracking Jun 11, 2019
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 13, 2019
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 13, 2019
@mengqiy
Copy link
Member Author

mengqiy commented Jun 13, 2019

PTAL

Copy link
Contributor

@droot droot left a comment

Choose a reason for hiding this comment

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

I have a few minor comments, otherwise looks good.

docs/book/src/reference/admission-webhook.md Outdated Show resolved Hide resolved
docs/book/src/reference/admission-webhook.md Outdated Show resolved Hide resolved
docs/book/src/reference/admission-webhook.md Outdated Show resolved Hide resolved
spec := c.Spec
fldPath := field.NewPath("spec")

if len(spec.Schedule) == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps put an example of actually parsing the schedule and seeing if it has an error?

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 13, 2019
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 15, 2019
@mengqiy
Copy link
Member Author

mengqiy commented Jun 17, 2019

PTAL

@mengqiy mengqiy added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 18, 2019
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 18, 2019
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 18, 2019
@mengqiy
Copy link
Member Author

mengqiy commented Jun 18, 2019

Rebased to resolve conflict.
PTAL

Copy link
Contributor

@DirectXMan12 DirectXMan12 left a comment

Choose a reason for hiding this comment

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

a bunch a minor grammar or wording comments, and a couple of structure comments

docs/book/src/SUMMARY.md Outdated Show resolved Hide resolved
docs/book/src/SUMMARY.md Outdated Show resolved Hide resolved
docs/book/src/reference/webhook-overview.md Outdated Show resolved Hide resolved
docs/book/src/reference/admission-webhook.md Outdated Show resolved Hide resolved
docs/book/src/reference/admission-webhook.md Outdated Show resolved Hide resolved
docs/book/src/reference/admission-webhook.md Outdated Show resolved Hide resolved
docs/book/src/reference/admission-webhook.md Outdated Show resolved Hide resolved
@mengqiy
Copy link
Member Author

mengqiy commented Jul 3, 2019

PTAL
I will squash commits before merging.

@c10l
Copy link

c10l commented Jul 5, 2019

Shouldn't automated tests (ginkgo/gomega) be a part of this?

@mengqiy
Copy link
Member Author

mengqiy commented Jul 8, 2019

@cassianoleal kubebuilder v2 ATM doesn't scaffold much testing files, since we are trying to figure out what is the best way to scaffold tests. We do have plan to support it soon after we have resolve our v2 release blockers :)

@mengqiy mengqiy removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 9, 2019
@mengqiy
Copy link
Member Author

mengqiy commented Jul 9, 2019

This should be ready to go.

Copy link
Contributor

@droot droot left a comment

Choose a reason for hiding this comment

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

Looks good. Noticed config not being rendered properly on the kind reference.

docs/book/src/reference/kind.md Outdated Show resolved Hide resolved
docs/book/src/reference/kind.md Outdated Show resolved Hide resolved
@mengqiy
Copy link
Member Author

mengqiy commented Jul 10, 2019

Ready to ship.

@droot droot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 11, 2019
@k8s-ci-robot k8s-ci-robot merged commit 450508e into kubernetes-sigs:master Jul 11, 2019
2.0.0 cross-project tracking automation moved this from Code Awaiting Review to Done Jul 11, 2019
@mengqiy mengqiy deleted the webhookdoc branch July 12, 2019 21:10
@mengqiy
Copy link
Member Author

mengqiy commented Jul 12, 2019

Thanks for raising this.
We have noticed this yesterday. We plan to temporarily drop it from the example #853.
We will add it back when we fix it in controller-tools.

@mengqiy
Copy link
Member Author

mengqiy commented Jul 13, 2019

filed kubernetes-sigs/controller-tools#256 to track

@mengqiy
Copy link
Member Author

mengqiy commented Jul 15, 2019

@gautamkmr #857 fixed it.

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/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Come up with webhook developer workflow to test it locally
6 participants