Remove capture shortcodes & warn on future use.

[sftim]

After PR #21359 merged, some more capture shortcodes have crept in.

This PR

• removes the extra capture shortcodes
• emits a warning if someone uses a capture shortcode

I chose a warning rather than error to reduce the risk of a misguided merge breaking the site. There's no lint about broken shortcodes, and the CI tests the code from the fork, rather than testing the result of a simulated merge into master.

[sftim]

Related to issue #20335

[sftim]

Also, this is across localizations - @kbhawkey / @onlydole (as tech leads) could you take a look?

[netlify]

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

Built with commit 325ba87

https://deploy-preview-22175--kubernetes-io-master-staging.netlify.app

[kbhawkey]

@sftim,
One thing, is it time to remove the capture shortcode instead of adding a warning?
Did these changes create a non-failing deploy preview? I guess so.
Is there is a way to add a pre-check build to the deploy build that marks the build as fail (should fail if the capture shortcode is removed) plus adds a hold on the PR?

[kbhawkey]

/kind cleanup

[sftim]

One thing, is it time to remove the capture shortcode instead of adding a warning?

@kbhawkey I'm worried about the blast radius of a localization team merging a new PR that breaks the website because a page uses a missing shortcode. This way, the updated page doesn't render correctly but the build remains working.

I'd like to get Netlify to simulate a merge and then build a preview, which would detect that issue and block a merge, but making that change happen is not an easy ask.

[sftim]

We can implement additional checks (eg using GitHub Actions) that create a preview merge and then try to run Hugo. That seems to me like it's duplicating what Netlify already does, but it could be useful.

[kbhawkey]

Instead, removing the capture shortcode will throw a build error.

[sftim]

removing the capture shortcode will throw a build error

I'm wary of having that error happen after a merge to master - it means prompt feedback but also more of a pain to fix.

[kbhawkey]

@sftim, What do you think about adding more information to the warning.
It would be helpful to provide the capture_id in the warning message.
I could see leaving lines 1-6 and adding the capture_id.
Also, add the prefix to identify with the page path:

{{ $_hugo_config := `{ "version": 1 }`}}
{{- $id := .Get 0 -}}
{{- if not $id -}}
{{- errorf "missing id in capture" -}}
{{- end -}}
{{- $capture_id := printf "capture %s" $id -}}
<!--
{{- .Page.Scratch.Set $capture_id .Inner -}}
-->
{{ warnf "Invalid shortcode: in %s, %q" $capture_id (relLangURL .Page.Path) }}

[zparnold]

Seems fine to me, probably need some other tech lead to approve whilst I get back up to speed.
/lgtm

[kbhawkey]

@sftim, Things look good. Would you remove the comment? I'd like to see a clean build/deploy preview
before merging. Thanks!

[sftim]

Removed!

[sftim]

Similar to PR #22321

[kbhawkey]

/lgtm
This will help.

[kbhawkey]

nit: The Scratch set could be removed?

[kbhawkey]

Let's move this forward.
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kbhawkey

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:

• OWNERS [kbhawkey]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[sftim]

“We got one”. This warning helped me find an issue and submit PR #22397.

[kbhawkey]

Yes, I saw it in testing this PR.