fix: clean up feature shortcodes in .html files and resolve orphaned … - #4689
fix: clean up feature shortcodes in .html files and resolve orphaned …#4689Sivasankaran25 wants to merge 6 commits into
Conversation
github-actions
Bot
added
kind/cleanup
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Updates the release process and the feature-shortcode-update tool to handle resolved feature shortcodes across more file types and to apply version-aware cleanup logic.
Changes:
- Expand
feature-shortcode-updateto scan both.mdand.html, using regex + version comparison to remove/unwrap feature shortcodes. - Update the release issue template and Makefile comments to reflect the new intended behavior for expiry/publish shortcodes.
- Add helper functions for shortcode matching and version comparison.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| docs/governance/templates/release_issue.md | Updates release checklist guidance for how shortcodes should be removed/unstyled across docs. |
| build/scripts/feature-shortcode-update/main.go | Implements regex-based shortcode detection, version comparisons, and .html inclusion. |
| build/includes/website.mk | Updates target documentation to match the intended shortcode cleanup behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if ext == ".md" { | ||
| if v, ok := matchOpen(line, publishOpenPercentRe, publishOpenAngleRe); ok && versionLTE(v, targetVersion) { | ||
| inPublishBlock = true | ||
| modified = true | ||
| continue | ||
| } | ||
| } |
There was a problem hiding this comment.
Agreed there's a mismatch, but I'd resolve it the other way round — fix the docs, keep the .md-only restriction. Removing it would break the release.
site/gen-api-docs.sh:48 locates the previous version's API docs by scanning for the publish shortcode:
awk '/\ feature\ publishVersion/{flag=1;next}/\ \/feature/{flag=0}flag' $FILE > $OLDIf the cleanup unwrapped publishVersion in agones_crd_api_reference.html, that awk would produce an empty $OLD, the generator would rewrite the file, and make test-gen-api-docs would fail with the misleading "API docs are out of date" error. #4684 calls this out explicitly. I confirmed the current behaviour is correct with a synthetic fixture — expiry half removed, publish wrapper preserved:
+++
title="Agones Kubernetes API"
+++
{{% feature publishVersion="1.61.0" %}}
<p>NEW DOCS</p>
{{% /feature %}}
So the two doc strings are what need correcting. Suggested wording —
build/includes/website.mk:
# For resolved shortcodes (version <= the release being cut) in site/content/en/docs:
# - expiryVersion: remove the tags and the wrapped content (.md and .html)
# - publishVersion: remove the tags, keep the content (.md only — .html keeps its
# publish wrapper, which gen-api-docs.sh uses to locate the previous API docs)docs/governance/templates/release_issue.md:40 — this one matters more, because it's the checklist someone follows under release pressure. If it says publishVersion is unwrapped in .html and they then see {{% feature publishVersion="..." %}} still sitting at the top of the CRD reference, the natural reaction is to delete it by hand and break test-gen-api-docs:
Run
make feature-shortcode-update version={version}to resolve everyfeatureshortcode naming a version<= {version}insite/content/en/docs:expiryVersionblocks are removed along with their content (both.mdand.html), andpublishVersionblocks are unwrapped, keeping their content (.mdonly). ThepublishVersionwrapper inagones_crd_api_reference.htmlis intentionally left in place —gen-api-docs.shuses it to locate the previous version's API docs, so removing it will breakmake test-gen-api-docs.
One related thought on the code: the exemption is keyed on the extension rather than on the one file that needs it. agones_crd_api_reference.html is the only .html under site/content/en/docs today, so a second one would get its publishVersion silently orphaned forever — the exact failure mode this PR exists to fix, just relocated. Either scope it to the filename, or keep the extension gate and log.Printf a warning when a resolved publishVersion is skipped in an .html file, so it surfaces in the release output.
| if inExpiryBlock { | ||
| // Drop every line inside a resolved expiryVersion block, including its own closing tag. | ||
| if matchFeatureClose(line) { | ||
| inExpiryBlock = false | ||
| } | ||
| modified = true | ||
| continue | ||
| } |
There was a problem hiding this comment.
Confirmed, and the outcome is worse than "gets stuck" — it destroys content that was never inside a block. Ran this on the branch:
$ cat site/content/en/docs/oneline.md
a
{{% feature expiryVersion="1.50.0" %}}x{{% /feature %}}
b
c
$ go run main.go -version=1.61.0
Processed file: site/content/en/docs/oneline.md
$ cat site/content/en/docs/oneline.md
a
b and c are gone. The open tag sets inExpiryBlock, the close on that same line is never examined, and every subsequent line is dropped to EOF. In a file that does have a later block, it instead swallows that block's closing tag and leaves stray markup.
One correction to the framing though: this is not a regression from this PR. I ran the identical fixture against main and got the same single line of output — the old preserveLines flag has exactly the same hole. So it's pre-existing, and there's no single-line usage anywhere in site/content/en/docs today (all five occurrences are on their own lines), which is why it has never bitten.
That makes it a "should fix" rather than a blocker for this PR, and @Sivasankaran25 it's reasonable to split it into a follow-up if you'd rather keep this change scoped. If you do fix it here, the regex refactor makes it cheap — check matchFeatureClose(line) on the opening line before setting the flag:
if v, ok := matchOpen(line, expiryOpenPercentRe, expiryOpenAngleRe); ok && versionLTE(v, targetVersion) {
inExpiryBlock = !matchFeatureClose(line)
modified = true
continue
}Note the publish case needs different handling, not the same line of code — see the sibling thread.
| if v, ok := matchOpen(line, expiryOpenPercentRe, expiryOpenAngleRe); ok && versionLTE(v, targetVersion) { | ||
| inExpiryBlock = true | ||
| modified = true | ||
| continue | ||
| } |
There was a problem hiding this comment.
Same finding as the thread on the expiry branch above — I've put the reproduction and the "this also affects main, so it's pre-existing" evidence there to keep it in one place.
Worth flagging that the publish case does not fail the same way, so "apply the same reasoning" needs a different fix. For a single-line {{% feature publishVersion="1.5.0" %}}text{{% /feature %}}, the whole line is dropped by the continue, so the wrapped text is lost even though publish blocks are supposed to keep their content — and then inPublishBlock stays set and consumes the next unrelated closing tag. Unwrapping in place is what's actually wanted here, i.e. strip the tags from the line and keep the remainder, rather than just clearing the flag.
| toInt := func(s string) int { | ||
| n, _ := strconv.Atoi(strings.TrimSpace(s)) | ||
| return n | ||
| } | ||
|
|
||
| for i := 0; i < len(vParts) || i < len(tParts); i++ { | ||
| var vn, tn int | ||
| if i < len(vParts) { | ||
| vn = toInt(vParts[i]) | ||
| } | ||
| if i < len(tParts) { | ||
| tn = toInt(tParts[i]) |
There was a problem hiding this comment.
Agreed, and I'd call this the one blocking item on the PR. It isn't only a misclassification — coercing to 0 makes an unparseable version look older than the release, so the block and its content get deleted:
$ cat site/content/en/docs/t.md
intro
{{% feature expiryVersion="1.x" %}}
FUTURE CONTENT DO NOT DELETE
{{% /feature %}}
outro
$ go run main.go -version=1.61.0
warning: could not parse version segment "x" in "1.x", treating as 0
Processed file: site/content/en/docs/t.md
$ cat site/content/en/docs/t.md
intro
outro
The realistic trigger isn't -rc1 suffixes, it's a plain typo. expiryVersion="v1.61.0" does the same thing: Atoi("v1") fails, first segment compares 0 < 1, block removed. So one stray character in a shortcode silently drops documentation during a release cut, and the only signal is a log.Printf line buried in the output of a tool that walks the whole docs tree.
On "failing fast" — I'd lean toward failing closed rather than failing fast: return an error from versionLTE, and at the call site log which file and shortcode were skipped and leave the block in place.
func versionLTE(v, target string) (bool, error) {
// return an error on the first segment that fails Atoi
}Leaving a stale shortcode in place is recoverable — someone notices it next release, which is exactly the class of thing this PR is fixing. Deleting content is not, and make feature-shortcode-update is run mid-release when the diff is competing for attention with a dozen other checklist items. Hard-failing the whole run is also defensible, but it means one typo blocks the release cut on a cleanup step.
| scanner := bufio.NewScanner(file) | ||
| modifiedContent := removeBlocks(scanner, *version) | ||
| modifiedContent := removeBlocks(scanner, *version, ext) |
There was a problem hiding this comment.
Agreed on both halves, with a note on severity.
Not currently reachable — the longest line anywhere under site/content/en/docs is 1358 chars (in Getting Started/create-webhook-fleetautoscaler.md), and agones_crd_api_reference.html is generated by gen-crd-api-reference-docs with one tag per line. So this is a latent issue rather than a live break.
The reason it's still worth fixing here is that the failure isn't just "partial processing" — the partial result gets written back over the file. If the scan aborts after modified has already been set by an earlier resolved block, main takes the truncated string and os.Creates the file with it. Silent data loss with a zero exit code.
scanner.Buffer(...) plus scanner.Err() covers it; the important part is that the error reaches main and short-circuits before the os.Create on line 81, which means removeBlocks returning (string, error).
| modifiedContent := removeBlocks(scanner, *version, ext) | ||
|
|
||
| // Only write the modified content back to the .md file if there are changes | ||
| if modifiedContent != "" { |
There was a problem hiding this comment.
Confirmed reachable. A file whose entire contents are a resolved block comes out unchanged:
$ cat site/content/en/docs/only.md
{{% feature expiryVersion="1.50.0" %}}
all of it
{{% /feature %}}
$ go run main.go -version=1.61.0
There are no files with feature expiryVersion or publishVersion shortcodes
$ cat site/content/en/docs/only.md
{{% feature expiryVersion="1.50.0" %}}
all of it
{{% /feature %}}
The shortcode survives the cleanup silently, and the log line actively says the opposite of what happened.
In practice every .md under site/content/en/docs has front matter, so a fully-empty result is unlikely today. But the sentinel-value conflation is worth removing regardless, and removeBlocks is likely to grow an error return anyway for the scanner.Err() thread — so (string, bool, error) or a small result struct lands both fixes together.
|
Build Failed 😭 Build Id: daeb3de5-88b7-43cf-9c4d-0313a6f2496e Status: FAILURE To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
|
Build Succeeded 🥳 Build Id: 98b86f3f-c464-4f59-8cd6-8e59c295f89d The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version: |
|
Build Succeeded 🥳 Build Id: 30c5b6ba-9c29-440f-ada5-8b02581896ae The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version: |
| func removeBlocks(scanner *bufio.Scanner, version string) string { | ||
| // removeBlocks assumes feature shortcodes never nest — a {{%|{{< /feature %}}|>}} always | ||
| // closes the single innermost open block, tracked via inExpiryBlock/inPublishBlock below. | ||
| func removeBlocks(scanner *bufio.Scanner, targetVersion, ext string) string { |
There was a problem hiding this comment.
This script has no main_test.go, recommend writing a new test as this PR adds a couple edge cases. There's a number of main_test.go in build/scripts, so there's a pattern to follow.
Cases worth pinning down, most of which came out of the Copilot threads on this PR:
versionLTE:1.9.0vs1.10.0(the numeric-vs-string case this PR fixes), equal versions, differing segment counts (1.61vs1.61.0), and whatever you decide unparseable input should do.removeBlockson.md: resolved expiry removed with content; resolved publish unwrapped with content kept; a future-version block of each kind left completely alone.removeBlockson.html: expiry removed, publish wrapper preserved — this is the one that keepstest-gen-api-docsgreen, so it's worth locking down explicitly.- The escaped
{{%/* feature ... */%}}form is not rewritten. - Both
{{% %}}and{{< >}}delimiter styles.
| if ext == ".md" { | ||
| if v, ok := matchOpen(line, publishOpenPercentRe, publishOpenAngleRe); ok && versionLTE(v, targetVersion) { | ||
| inPublishBlock = true | ||
| modified = true | ||
| continue | ||
| } | ||
| } |
There was a problem hiding this comment.
Agreed there's a mismatch, but I'd resolve it the other way round — fix the docs, keep the .md-only restriction. Removing it would break the release.
site/gen-api-docs.sh:48 locates the previous version's API docs by scanning for the publish shortcode:
awk '/\ feature\ publishVersion/{flag=1;next}/\ \/feature/{flag=0}flag' $FILE > $OLDIf the cleanup unwrapped publishVersion in agones_crd_api_reference.html, that awk would produce an empty $OLD, the generator would rewrite the file, and make test-gen-api-docs would fail with the misleading "API docs are out of date" error. #4684 calls this out explicitly. I confirmed the current behaviour is correct with a synthetic fixture — expiry half removed, publish wrapper preserved:
+++
title="Agones Kubernetes API"
+++
{{% feature publishVersion="1.61.0" %}}
<p>NEW DOCS</p>
{{% /feature %}}
So the two doc strings are what need correcting. Suggested wording —
build/includes/website.mk:
# For resolved shortcodes (version <= the release being cut) in site/content/en/docs:
# - expiryVersion: remove the tags and the wrapped content (.md and .html)
# - publishVersion: remove the tags, keep the content (.md only — .html keeps its
# publish wrapper, which gen-api-docs.sh uses to locate the previous API docs)docs/governance/templates/release_issue.md:40 — this one matters more, because it's the checklist someone follows under release pressure. If it says publishVersion is unwrapped in .html and they then see {{% feature publishVersion="..." %}} still sitting at the top of the CRD reference, the natural reaction is to delete it by hand and break test-gen-api-docs:
Run
make feature-shortcode-update version={version}to resolve everyfeatureshortcode naming a version<= {version}insite/content/en/docs:expiryVersionblocks are removed along with their content (both.mdand.html), andpublishVersionblocks are unwrapped, keeping their content (.mdonly). ThepublishVersionwrapper inagones_crd_api_reference.htmlis intentionally left in place —gen-api-docs.shuses it to locate the previous version's API docs, so removing it will breakmake test-gen-api-docs.
One related thought on the code: the exemption is keyed on the extension rather than on the one file that needs it. agones_crd_api_reference.html is the only .html under site/content/en/docs today, so a second one would get its publishVersion silently orphaned forever — the exact failure mode this PR exists to fix, just relocated. Either scope it to the filename, or keep the extension gate and log.Printf a warning when a resolved publishVersion is skipped in an .html file, so it surfaces in the release output.
| if inExpiryBlock { | ||
| // Drop every line inside a resolved expiryVersion block, including its own closing tag. | ||
| if matchFeatureClose(line) { | ||
| inExpiryBlock = false | ||
| } | ||
| modified = true | ||
| continue | ||
| } |
There was a problem hiding this comment.
Confirmed, and the outcome is worse than "gets stuck" — it destroys content that was never inside a block. Ran this on the branch:
$ cat site/content/en/docs/oneline.md
a
{{% feature expiryVersion="1.50.0" %}}x{{% /feature %}}
b
c
$ go run main.go -version=1.61.0
Processed file: site/content/en/docs/oneline.md
$ cat site/content/en/docs/oneline.md
a
b and c are gone. The open tag sets inExpiryBlock, the close on that same line is never examined, and every subsequent line is dropped to EOF. In a file that does have a later block, it instead swallows that block's closing tag and leaves stray markup.
One correction to the framing though: this is not a regression from this PR. I ran the identical fixture against main and got the same single line of output — the old preserveLines flag has exactly the same hole. So it's pre-existing, and there's no single-line usage anywhere in site/content/en/docs today (all five occurrences are on their own lines), which is why it has never bitten.
That makes it a "should fix" rather than a blocker for this PR, and @Sivasankaran25 it's reasonable to split it into a follow-up if you'd rather keep this change scoped. If you do fix it here, the regex refactor makes it cheap — check matchFeatureClose(line) on the opening line before setting the flag:
if v, ok := matchOpen(line, expiryOpenPercentRe, expiryOpenAngleRe); ok && versionLTE(v, targetVersion) {
inExpiryBlock = !matchFeatureClose(line)
modified = true
continue
}Note the publish case needs different handling, not the same line of code — see the sibling thread.
| if v, ok := matchOpen(line, expiryOpenPercentRe, expiryOpenAngleRe); ok && versionLTE(v, targetVersion) { | ||
| inExpiryBlock = true | ||
| modified = true | ||
| continue | ||
| } |
There was a problem hiding this comment.
Same finding as the thread on the expiry branch above — I've put the reproduction and the "this also affects main, so it's pre-existing" evidence there to keep it in one place.
Worth flagging that the publish case does not fail the same way, so "apply the same reasoning" needs a different fix. For a single-line {{% feature publishVersion="1.5.0" %}}text{{% /feature %}}, the whole line is dropped by the continue, so the wrapped text is lost even though publish blocks are supposed to keep their content — and then inPublishBlock stays set and consumes the next unrelated closing tag. Unwrapping in place is what's actually wanted here, i.e. strip the tags from the line and keep the remainder, rather than just clearing the flag.
| toInt := func(s string) int { | ||
| n, _ := strconv.Atoi(strings.TrimSpace(s)) | ||
| return n | ||
| } | ||
|
|
||
| for i := 0; i < len(vParts) || i < len(tParts); i++ { | ||
| var vn, tn int | ||
| if i < len(vParts) { | ||
| vn = toInt(vParts[i]) | ||
| } | ||
| if i < len(tParts) { | ||
| tn = toInt(tParts[i]) |
There was a problem hiding this comment.
Agreed, and I'd call this the one blocking item on the PR. It isn't only a misclassification — coercing to 0 makes an unparseable version look older than the release, so the block and its content get deleted:
$ cat site/content/en/docs/t.md
intro
{{% feature expiryVersion="1.x" %}}
FUTURE CONTENT DO NOT DELETE
{{% /feature %}}
outro
$ go run main.go -version=1.61.0
warning: could not parse version segment "x" in "1.x", treating as 0
Processed file: site/content/en/docs/t.md
$ cat site/content/en/docs/t.md
intro
outro
The realistic trigger isn't -rc1 suffixes, it's a plain typo. expiryVersion="v1.61.0" does the same thing: Atoi("v1") fails, first segment compares 0 < 1, block removed. So one stray character in a shortcode silently drops documentation during a release cut, and the only signal is a log.Printf line buried in the output of a tool that walks the whole docs tree.
On "failing fast" — I'd lean toward failing closed rather than failing fast: return an error from versionLTE, and at the call site log which file and shortcode were skipped and leave the block in place.
func versionLTE(v, target string) (bool, error) {
// return an error on the first segment that fails Atoi
}Leaving a stale shortcode in place is recoverable — someone notices it next release, which is exactly the class of thing this PR is fixing. Deleting content is not, and make feature-shortcode-update is run mid-release when the diff is competing for attention with a dozen other checklist items. Hard-failing the whole run is also defensible, but it means one typo blocks the release cut on a cleanup step.
| scanner := bufio.NewScanner(file) | ||
| modifiedContent := removeBlocks(scanner, *version) | ||
| modifiedContent := removeBlocks(scanner, *version, ext) |
There was a problem hiding this comment.
Agreed on both halves, with a note on severity.
Not currently reachable — the longest line anywhere under site/content/en/docs is 1358 chars (in Getting Started/create-webhook-fleetautoscaler.md), and agones_crd_api_reference.html is generated by gen-crd-api-reference-docs with one tag per line. So this is a latent issue rather than a live break.
The reason it's still worth fixing here is that the failure isn't just "partial processing" — the partial result gets written back over the file. If the scan aborts after modified has already been set by an earlier resolved block, main takes the truncated string and os.Creates the file with it. Silent data loss with a zero exit code.
scanner.Buffer(...) plus scanner.Err() covers it; the important part is that the error reaches main and short-circuits before the os.Create on line 81, which means removeBlocks returning (string, error).
| modifiedContent := removeBlocks(scanner, *version, ext) | ||
|
|
||
| // Only write the modified content back to the .md file if there are changes | ||
| if modifiedContent != "" { |
There was a problem hiding this comment.
Confirmed reachable. A file whose entire contents are a resolved block comes out unchanged:
$ cat site/content/en/docs/only.md
{{% feature expiryVersion="1.50.0" %}}
all of it
{{% /feature %}}
$ go run main.go -version=1.61.0
There are no files with feature expiryVersion or publishVersion shortcodes
$ cat site/content/en/docs/only.md
{{% feature expiryVersion="1.50.0" %}}
all of it
{{% /feature %}}
The shortcode survives the cleanup silently, and the log line actively says the opposite of what happened.
In practice every .md under site/content/en/docs has front matter, so a fully-empty result is unlikely today. But the sentinel-value conflation is worth removing regardless, and removeBlocks is likely to grow an error return anyway for the scanner.Err() thread — so (string, bool, error) or a small result struct lands both fixes together.
|
Build Succeeded 🥳 Build Id: 9d7f8e6b-cf91-4f4e-b2b4-cd586b062a9f The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version: |
|
Build Failed 😭 Build Id: 64819025-6289-4262-abe3-6e496e5b58c0 Status: FAILURE To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
|
/gcbrun |
|
Build Failed 😭 Build Id: 25d48685-b848-4445-93aa-aa644e78e89d Status: FAILURE To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
|
/gcbrun |
|
Build Succeeded 🥳 Build Id: 08062ae4-bf81-4c97-b250-d0dba400d332 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version: |
|
Build Failed 😭 Build Id: cabb52b5-4acd-43ef-b8de-dcc9d4913fa9 Status: FAILURE To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
…versions
What type of PR is this?
/kind cleanup
What this PR does / Why we need it:
Which issue(s) this PR fixes:
Closes #4684
Did you use AI tools in preparing this PR?:
Y/N
Special notes for your reviewer: