-
Notifications
You must be signed in to change notification settings - Fork 14.4k
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
Update links in extensible-admission-controllers.md #26060
Conversation
Welcome @Abirdcfly! |
/assign @kbhawkey |
✔️ Deploy preview for kubernetes-io-master-staging ready! 🔨 Explore the source changes: 257794c 🔍 Inspect the deploy logs: https://app.netlify.com/sites/kubernetes-io-master-staging/deploys/5ffeaff90e3e7200088a2e33 😎 Browse the preview: https://deploy-preview-26060--kubernetes-io-master-staging.netlify.app |
Hello @Abirdcfly . Thanks for opening this pull request. |
/retitle Update links in extensible-admission-controllers.md |
/lgtm |
LGTM label has been added. Git tree hash: 717bab5bab14a39d5829e756d277ad6375879cd3
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Aut0R3V The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Pointing to old versions of source code is not ideal. However, pointing to the "master" branch is not a good idea either because the "master" is a moving target. |
New changes are detected. LGTM label has been removed. |
It is common to pointing to source code. And If we want to explain clearly, it is necessary to show the source code. I have change master branch to |
/assign @kbhawkey |
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 like the idea of this PR. I'm worried though that these changes make things more fragile - see inline feedback.
@@ -67,7 +67,7 @@ See the [webhook request](#request) section for details on the data sent to webh | |||
See the [webhook response](#response) section for the data expected from webhooks. | |||
|
|||
The example admission webhook server leaves the `ClientAuth` field | |||
[empty](https://github.com/kubernetes/kubernetes/blob/v1.13.0/test/images/webhook/config.go#L47-L48), | |||
[empty](https://github.com/kubernetes/kubernetes/blob/{{< param "githubbranch" >}}/test/images/agnhost/webhook/config.go#L38-L39), |
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.
If hyperlinking to a specific line, it's better to pin to a named branch (the latest release is a good choice). Otherwise if that file changes the line might end up reading something else.
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 have check these specific line in version 1.17 to 1.20 it points to same items. 😊 If we change source code, I think it is a good idea to change website too.
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.
Hi @Abirdcfly .
It would be helpful to get this link updated.
Since the parameter githubbranch
resolves to master
, would you change this section of the link
to the current version, 1.20.0
, or possibly the variable, fullversion
?
This link may change in the future?
ping @kbhawkey |
Hi @Abirdcfly . What do you think about updating the version in the links to the latest version, |
It work. There are two choices: |
Pointing to master/main is never a good choice to me. Please consider pointing to a tag for latest release. |
/close |
@tengqm: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
update link