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.

Sign up for GitHub

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

Jump to bottom

Resource name constraints (1) #19106

Merged
merged 1 commit into from
Feb 19, 2020
Merged

Resource name constraints (1) #19106

merged 1 commit into from
Feb 19, 2020

Conversation

tengqm
Copy link
Contributor

@tengqm tengqm commented Feb 13, 2020

xref: #17969, #19099, #18746

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. language/en Issues or PRs related to English language sig/docs Categorizes an issue or PR as relevant to SIG Docs. labels Feb 13, 2020
@netlify
Copy link

netlify bot commented Feb 13, 2020 •
edited
Loading

Deploy preview for kubernetes-io-master-staging ready!

Built with commit e7d6a34

https://deploy-preview-19106--kubernetes-io-master-staging.netlify.com

@kbhawkey
Copy link
Contributor

kbhawkey commented Feb 13, 2020 •
edited
Loading

Page previews:
https://deploy-preview-19106--kubernetes-io-master-staging.netlify.com/docs/concepts/overview/working-with-objects/names/#dns-subdomain-names

https://deploy-preview-19106--kubernetes-io-master-staging.netlify.com/docs/reference/access-authn-authz/extensible-admission-controllers/#webhook-configuration

@tengqm tengqm force-pushed the name-constraints branch 2 times, most recently from 9032a91 to 82d6723 Compare February 14, 2020 03:00
This was referenced Feb 14, 2020
Merged
Merged
This was referenced Feb 14, 2020
Merged
Merged
@kbhawkey
Copy link
Contributor

@tengqm , I am reading through the updates. I just noticed that the title
of the page with the changes is called Names and the main h2 heading is also called
Names. What do you think about updating the title or updating the first heading?

https://deploy-preview-19106--kubernetes-io-master-staging.netlify.com/docs/concepts/overview/working-with-objects/names/#names

kbhawkey
kbhawkey reviewed Feb 15, 2020
View reviewed changes
content/en/docs/concepts/overview/working-with-objects/names.md
@@ -42,6 +69,7 @@ spec:
- containerPort: 80
```


Copy link
Contributor

Choose a reason for hiding this comment

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

Another nit:
Is the Pod manifest a good example of naming conventions or could you provide some different examples of
Kubernetes resource names.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pod is the basic building block in Kubernetes, so I think keeping it here makes sense.
We can certainly provide more examples here or somewhere else but I don't see it necessary.
In follow-up PRs, there are references to these resource name constraints.

@tengqm
Copy link
Contributor Author

tengqm commented Feb 16, 2020

the title of the page with the changes is called Names and the main h2 heading is also called
Names.

Em... that is not ideal. Changing either one may break the links or bookmarks people have. For example, if we change either one, the existing references to /docs/concepts/overview/working-with-objects/names or /docs/concepts/overview/working-with-objects/names#names will break. So I don't see it necessary. Maybe we can revisit this when we have a checker for dangling links.

sftim
sftim reviewed Feb 16, 2020
View reviewed changes
Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

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

@tengqm - how about referring to https://tools.ietf.org/html/rfc2181#section-11 for the definition of DNS labels and names?

@sftim
Copy link
Contributor

sftim commented Feb 16, 2020

I think it's OK to risk breaking https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names as it's the first content heading. If it's important to preserve it, it's feasible to add an element to the page at pretty much the right place (eg just before the renamed heading), and set its id attribute to names.

@sftim
Copy link
Contributor

sftim commented Feb 16, 2020

We could rename the page to “Object Names and UIDs", then add a redirect to its new URL.

kbhawkey
kbhawkey reviewed Feb 19, 2020
View reviewed changes
content/en/docs/concepts/overview/working-with-objects/names.md
### DNS Label Names

There are some resource names are required to conform to the DNS
label standard as defined in [RFC 1123](https://tools.ietf.org/html/rfc1123).
Copy link
Contributor

Choose a reason for hiding this comment

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

@tengqm , changes look good. One nit, reword line 42. What do you think about these changes:
Some resources require their names to conform to the DNS label standard ...
Some resource types require their names to follow the DNS label standard ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@kbhawkey
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 19, 2020
sftim
sftim approved these changes Feb 19, 2020
View reviewed changes
Copy link
Contributor

@sftim sftim 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 to me, too!
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sftim

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 Feb 19, 2020
@k8s-ci-robot k8s-ci-robot merged commit ceccbc0 into kubernetes:master Feb 19, 2020
zacharysarah pushed a commit to zacharysarah/website that referenced this pull request Feb 21, 2020
@tengqm
Resource name constraints (1) (kubernetes#19106)
c17137a 
xref: kubernetes#17969, kubernetes#19099, kubernetes#18746
wawa0210 pushed a commit to wawa0210/website that referenced this pull request Mar 2, 2020
@tengqm @wawa0210
Resource name constraints (1) (kubernetes#19106)
eaf6d41 
xref: kubernetes#17969, kubernetes#19099, kubernetes#18746
@tengqm tengqm deleted the name-constraints branch March 6, 2020 03:51
VineethReddy02 pushed a commit to VineethReddy02/website that referenced this pull request Mar 22, 2020
@tengqm @VineethReddy02
Resource name constraints (1) (kubernetes#19106)
33c5784 
xref: kubernetes#17969, kubernetes#19099, kubernetes#18746
@pacoxu pacoxu mentioned this pull request Dec 14, 2020
Closed
@sftim sftim mentioned this pull request Mar 3, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kbhawkey kbhawkey kbhawkey left review comments

@sftim sftim sftim approved these changes

@smarterclayton smarterclayton Awaiting requested review from smarterclayton

@thockin thockin Awaiting requested review from thockin

Assignees

@kbhawkey kbhawkey

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. language/en Issues or PRs related to English language lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/docs Categorizes an issue or PR as relevant to SIG Docs. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tengqm @kbhawkey @sftim @k8s-ci-robot