Link to Latest CNCF CoC Instead of Commit Hash #50850
Conversation
k8s-ci-robot
added
cncf-cla: yes
k8s-ci-robot
added
language/es
✅ Pull request preview available for checkingBuilt without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
@avanish23 thank you for your work! Following the workflow we have here for website localisations, we'll need to separate all localisation-specific changes into individual PRs, each for the corresponding language. Please, leave the English-related changes only for this PR 🙏 You (or someone else) can create the follow-up PRs for each localisation after this PR is merged. |
|
Thanks for reviewing the PR @shurup |
I think we can also help you with this, so that it'll be easier for you to preview the changes before bringing them to the PR. Feel free to open a relevant issue where you can provide your output with the failing build, and mention me there. |
|
Thanks for this work. We need a different approach before we should merge the change:
Look at the /area web-development |
k8s-ci-robot
added
the
area/web-development
|
Thanks @lmktfy for your feedback, I will look into your suggestions. |
k8s-ci-robot
added
size/XS
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
1 similar comment
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
k8s-ci-robot
added
size/S
|
Thanks @lmktfy for your feedback, I will look into your suggestions. |
|
@shurup @lmktfy Attached screenshot Kindly review the PR again. |
|
Thank you for your fixes! LGTM now. However, I wonder why @lmktfy mentioned:
… in the related issue #49595 (comment). Currently, we have this link, despite the content of this document is inserted below. Perhaps we're still missing something, so I'd prefer to wait for him to give the final approval. /remove-language es fr id it ja ko pt ru |
k8s-ci-robot
removed
language/es
There was a problem hiding this comment.
I'm afraid to say that I think the website will be better, overall, if we do not merge this change.
• The PR title is wrong; this is transfusion, not hyperlinking.
• I have significant doubts that the change, as it stands, leaves the site easy enough to maintain. Some of what I see looks "fragile": small changes in other places could have bigger consequences.
• The error handling isn't right for our pridui website.
There was a problem hiding this comment.
For a production website build, a failed fetch is a build failure, but for local development (with eg make container-serve) a failed fetch should only be a warning.
Please change this code to achieve that.
Look at the warnf and errorf functions (and for layouts that already call the right one depending on context) for a hint about how to do this
| remote-coc shortcode: Fetches remote Markdown, cleans up 'Other languages' section, renders as HTML | ||
| */}} | ||
|
|
||
| {{ $url := "https://raw.githubusercontent.com/cncf/foundation/main/code-of-conduct.md" }} |
There was a problem hiding this comment.
Does this work for all locales (is not just English)? Will it work reliably?
| {{ $url := "https://raw.githubusercontent.com/cncf/foundation/main/code-of-conduct.md" }} | ||
| {{ with resources.GetRemote $url }} | ||
| {{ $raw := .Content }} | ||
| {{ $clean := replaceRE "(?ms)^Other languages available:\\s*\\n(?:- .+\\n)*- \\[Vietnamese/Tiếng Việt\\]\\(code-of-conduct-languages/vi\\.md\\)\\n*" "" $raw }} |
There was a problem hiding this comment.
How do we know that these are the right strings to match on? If we aren't sure, this change may make the Kubernetes website harder to maintain.
| {{ $html := $clean | markdownify }} | ||
| {{ $html }} | ||
| {{ else }} | ||
| {{ warnf "remote-coc shortcode: Failed to fetch remote resource from %s" $url }} |
There was a problem hiding this comment.
For a production build, this failure needs to be an outright error.
| {{ $html }} | ||
| {{ else }} | ||
| {{ warnf "remote-coc shortcode: Failed to fetch remote resource from %s" $url }} | ||
| <p><em>Content temporarily unavailable. Please try again later.</em></p> |
There was a problem hiding this comment.
This text should be localized, even if is kinky used for local previews.
There was a problem hiding this comment.
I am not confident that we can accept this change, but if we do, I think we could omit:
If you notice that this page is out of date, please [file an issue](https://github.com/kubernetes/website/issues/new/choose).
Description
Right now, the Kubernetes CoC inherits the CNCF CoC. We place the content of the CNCF CoC in a static folder and source it from there, which means that every time the CNCF CoC is updated, we need to raise a PR updating the CoC on the Kubernetes side in the static folders.
Issue
Now using the hugo function resources.GetRemote we can get the CNCF CoC content during the hugo build. No need to create new PRs everytime CNCF CoC is updated.
Closes: #49595