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

Warn on new use of param shortcode #46324

Merged
merged 1 commit into from May 16, 2024
Merged

Warn on new use of param shortcode #46324

merged 1 commit into from May 16, 2024
+13 −1

Conversation

sftim
Copy link
Contributor

@sftim sftim commented May 11, 2024 •
edited

[AIUI] we prefer not to use the param shortcode (we switched to a new approach in PR #40692)

Change that shortcode to warn if it's used other than for version (which we still support, although I recommend using the skew shortcode in its place).
As part of this:

  • switch Bengali to the new approach
  • fix a broken link in a blog article
  • change a couple of pages to use the skew shortcode

/area web-development

@k8s-ci-robot k8s-ci-robot added the area/web-development Issues or PRs related to the kubernetes.io's infrastructure, design, or build processes label May 11, 2024
@k8s-ci-robot k8s-ci-robot added the language/bn Issues or PRs related to Bengali language label May 11, 2024
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. sig/docs Categorizes an issue or PR as relevant to SIG Docs. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 11, 2024
sftim
sftim commented May 11, 2024
View reviewed changes
content/bn/docs/tasks/tools/install-kubectl-windows.md Outdated
@@ -24,12 +24,12 @@ kubectl এর সর্বশেষ সামঞ্জস্যপূর্ণ

### উইন্ডোজে কার্ল ব্যাবহার kubectl বাইনারি ইনস্টল করুন

1. [latest release {{< param "fullversion" >}}](https://dl.k8s.io/release/{{< param "fullversion" >}}/bin/windows/amd64/kubectl.exe) ডাউনলোড করুন।
1. [kubectl {{% skew currentPatchVersion %}}](https://dl.k8s.io/release/v{{% skew currentPatchVersion %}}/bin/windows/amd64/kubectl.exe) এর সর্বশেষ প্রকাশ ডাউনলোড করুন
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a machine-aided translation; I don't speak Bengali

@sftim sftim force-pushed the 20240511_drop_param_shortcode branch from ca0d165 to c370e2c Compare May 11, 2024 20:02
@netlify Netlify
Copy link

netlify bot commented May 11, 2024

✅ Pull request preview available for checking

Built without sensitive environment variables

Name Link
🔨 Latest commit ca0d165
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-io-main-staging/deploys/663fce245563d40008d5f31b
😎 Deploy Preview https://deploy-preview-46324--kubernetes-io-main-staging.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@netlify Netlify
Copy link

netlify bot commented May 11, 2024 •
edited

✅ Pull request preview available for checking

Built without sensitive environment variables

Name Link
🔨 Latest commit add8b37
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-io-main-staging/deploys/66451c5d6476d50008e214a0
😎 Deploy Preview https://deploy-preview-46324--kubernetes-io-main-staging.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

dipesh-rawat
dipesh-rawat reviewed May 12, 2024
View reviewed changes
Copy link
Member

@dipesh-rawat dipesh-rawat left a comment

Choose a reason for hiding this comment

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

Overall, the change looks good!
Just added an observation on the removal of shortcode.

LGTM

layouts/shortcodes/param.html Outdated
Copy link
Member

Choose a reason for hiding this comment

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

I'm surprised that removing the shortcode HTML file hasn't caused build errors, considering there are still instances in the repo where the <param> shortcode is used (example 1, example 2). I assume now the site is using the shortcode source retrieved from Docsy or its dependencies during the build process.

@sftim
Copy link
Contributor Author

sftim commented May 12, 2024

/retitle Drop param shortcodes, warn on new use

@k8s-ci-robot k8s-ci-robot changed the title Drop param shortcode Drop param shortcodes, warn on new use May 12, 2024
@sftim sftim marked this pull request as draft May 12, 2024 01:06
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 12, 2024
@sftim
Copy link
Contributor Author

sftim commented May 12, 2024

Do we want to merge this with lots of pages showing warnings, or split out the Bengali fixes, or something else?

@sftim sftim force-pushed the 20240511_drop_param_shortcode branch from c370e2c to 9e52ecc Compare May 12, 2024 01:13
@k8s-ci-robot k8s-ci-robot added area/blog Issues or PRs related to the Kubernetes Blog subproject language/en Issues or PRs related to English language size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 12, 2024
@sftim sftim force-pushed the 20240511_drop_param_shortcode branch from 9e52ecc to 4c68cd2 Compare May 12, 2024 01:16
@k8s-ci-robot k8s-ci-robot added area/release-eng Issues or PRs related to the Release Engineering subproject sig/release Categorizes an issue or PR as relevant to SIG Release. labels May 12, 2024
@sftim sftim force-pushed the 20240511_drop_param_shortcode branch 2 times, most recently from 3c98451 to 54e1a23 Compare May 12, 2024 01:34
@sftim sftim marked this pull request as ready for review May 12, 2024 01:34
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 12, 2024
@asem-hamid
Copy link
Contributor

bn fixes has been merged in this PR

@sftim sftim force-pushed the 20240511_drop_param_shortcode branch from b64847c to add8b37 Compare May 15, 2024 20:34
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 15, 2024
@sftim
Copy link
Contributor Author

sftim commented May 15, 2024

/retitle Warn on new use of param shortcode

@k8s-ci-robot k8s-ci-robot changed the title Fix legacy param shortcodes; warn on new use Warn on new use of param shortcode May 15, 2024
@sftim
Copy link
Contributor Author

sftim commented May 15, 2024

/remove-area blog release

@k8s-ci-robot k8s-ci-robot removed the area/blog Issues or PRs related to the Kubernetes Blog subproject label May 15, 2024
@k8s-ci-robot
Copy link
Contributor

@sftim: Those labels are not set on the issue: area/release

In response to this:

/remove-area blog release

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-sigs/prow repository.

sftim
sftim commented May 15, 2024
View reviewed changes
layouts/shortcodes/param.html
Comment on lines +1 to +4
{{- with $name -}}
{{- with ($.Page.Param .) }}{{ . }}{{ else }}{{ errorf "Param %q not found: %s" $name $.Position }}{{ end -}}
{{- else }}{{ errorf "Missing param key: %s" $.Position }}{{ end -}}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is Hugo's default param shortcode.

sftim
sftim commented May 15, 2024
View reviewed changes
layouts/shortcodes/param.html
Comment on lines +5 to +13
{{- $warningText := slice
"Deprecated param shortcode detected."
"The Kubernetes website does not / should not use param shortcodes."
( printf "Check %q" ( replaceRE "^/src/" "" . ) ) -}}
{{- if and (eq $.Site.LanguagePrefix "") (ne $name "version" ) -}}
{{- warnf (delimit $warningText " " ) -}}
{{- end -}}
{{- end -}}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is our warning mechanism.

@sftim
Copy link
Contributor Author

sftim commented May 15, 2024

Updated. This PR won't add warnings, but future pages that use param might make a new warning show up.

@sftim
Copy link
Contributor Author

sftim commented May 15, 2024

/hold cancel

The fixes to Bengali have merged.

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 15, 2024
@dipesh-rawat
Copy link
Member

/remove-language bn
/remove-area release-eng
/remove-sig release

@k8s-ci-robot k8s-ci-robot removed language/bn Issues or PRs related to Bengali language area/release-eng Issues or PRs related to the Release Engineering subproject sig/release Categorizes an issue or PR as relevant to SIG Release. labels May 15, 2024
dipesh-rawat
dipesh-rawat reviewed May 15, 2024
View reviewed changes
Copy link
Member

@dipesh-rawat dipesh-rawat left a comment

Choose a reason for hiding this comment

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

The changes look good!

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 15, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 6111cea251c00b94b9ed363b6347e2402e3472c7

@ydFu
Copy link
Member

ydFu commented May 16, 2024

/lgtm
This change can solve specific problems and will not involve files with cross-language permissions.

@salaxander
Copy link
Contributor

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: salaxander

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 May 16, 2024
@k8s-ci-robot k8s-ci-robot merged commit a840305 into kubernetes:main May 16, 2024
6 checks passed
@sftim sftim deleted the 20240511_drop_param_shortcode branch May 16, 2024 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dipesh-rawat dipesh-rawat dipesh-rawat left review comments

@asem-hamid asem-hamid asem-hamid requested changes

@divya-mohan0209 divya-mohan0209 Awaiting requested review from divya-mohan0209

@mitul3737 mitul3737 Awaiting requested review from mitul3737

@davidopp davidopp Awaiting requested review from davidopp

Assignees

@dipesh-rawat dipesh-rawat

@ydFu ydFu

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/web-development Issues or PRs related to the kubernetes.io's infrastructure, design, or build processes 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/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@sftim @dipesh-rawat @ydFu @ricardoamaro @asem-hamid @k8s-ci-robot @salaxander