refactor(starters): render the starter workspaces from Go instead of shell - #615
Merged
Conversation
viniciusdc
requested review from
aktech,
dcmcand,
marcelovilla,
oldsj and
tylerpotts
as code owners
August 25, 2026 16:10
marcelovilla
previously approved these changes
Aug 25, 2026
Member
There was a problem hiding this comment.
Thanks for addressing all the comments @viniciusdc! Approving now
This comment was actually meant for #594
marcelovilla
self-requested a review
August 25, 2026 16:51
marcelovilla
dismissed
their stale review
August 25, 2026 16:53
Dismissing my own review as it was meant for another PR
…shell
Replaces scripts/gen-starters.sh with cmd/starters. Output is byte-identical
to the shell version; the difference is what can go wrong and when it is
caught.
The shell script located values by line prefix, indentation included:
' email: '
' url: '
' path: '
That cannot distinguish the GitOps path: from any other four-space path: key,
so the script needed a guard requiring each prefix to match exactly one line,
and a restructure of examples/ was only caught once CI had built nic and
rendered a starter. Resolving each field as a YAML path instead
($.repository.existing.path) removes the ambiguity rather than defending
against it: a path either resolves or it fails, naming itself.
Three pieces of machinery go away with it - the exactly-one-match guard, the
sed dance that reattached a trailing comment to a replaced line, and the
hardcoded indentation.
Comments and blank lines still survive, which was the one argument for staying
in shell. The edit is applied to the source text rather than by marshalling the
parsed document back out: each value's token gives the line and column where
the value starts, so rewriting from that column leaves the rest of the line -
including a trailing comment - untouched. goccy/go-yaml is already the parser
on both sides of this: #603 made it the config parser and #583's placeholder
gate walks its AST.
Block scalars are rejected explicitly. Their token is the |/> indicator rather
than the body, so a newline check never fires and an in-place edit would leave
the following lines orphaned.
The tests are the point of the move: declared fields are resolved against the
real examples/*.yaml at go test time, so a renamed key fails on a laptop
instead of in CI, and the same-named-sibling case that the line-prefix match
got wrong is pinned directly.
viniciusdc
force-pushed
the
feat/starters-go-generator
branch
from
August 25, 2026 16:58
0aaa205 to
e665a0b
Compare
viniciusdc
commented
Aug 25, 2026
The three os.ReadFile calls take composed paths, which trips G304. Wrapped in filepath.Clean, matching how cmd/docgen handles the same rule on its writes. The test's read of a checked-in example is a fixed repo-relative path, so it carries an annotated nolint instead, as cmd/docgen/cli_test.go does.
marcelovilla
approved these changes
Aug 25, 2026
marcelovilla
left a comment
Member
There was a problem hiding this comment.
Thanks for this PR @viniciusdc! As I mentioned before, I think this approach is more robust. Tested locally and works as expected.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follows up @marcelovilla's review comment on #594: render the starters from a small Go command rather than a bash script, so the fragile parts are unit-testable.
What
scripts/gen-starters.shbecomescmd/starters. Output is byte-identical - I diffed the two generators' full output trees before deleting the script.The difference is what can go wrong, and when it is caught.
Why it is less fragile, concretely
The script located values by line prefix, indentation included:
That cannot tell the GitOps
path:from any other four-spacepath:key, which is why the script grew a guard requiring each prefix to match exactly one line - and a restructure ofexamples/only surfaced once CI had builtnicand rendered a starter.Resolving each field as a YAML path removes the ambiguity instead of defending against it:
"$.repository.existing.path"A path either resolves or it fails naming itself. Three pieces of machinery go with it: the exactly-one-match guard, the
seddance that reattached a trailing comment to a replaced line, and the hardcoded indentation.Comments and blank lines still survive
That was the one argument for staying in shell, and @marcelovilla checked it before raising this. The edit is applied to the source text rather than by marshalling the parsed document back out: each value's token gives the line and column where the value starts, so rewriting from that column leaves the rest of the line - trailing comment included - untouched.
goccy/go-yamlis already the parser on both sides of this - #603 made it the config parser, and #583's placeholder gate walks its AST - so this is the same library the validation half already uses, not a new dependency.Block scalars are rejected explicitly: their token is the
|/>indicator rather than the body, so a newline check never fires and an in-place edit would leave the following lines orphaned. That one cost me a red test before I understood it.Tests
The point of the move.
TestDeclaredFieldsResolveAgainstTheRealExamplesresolves every declared field against the actualexamples/*.yamlatgo testtime, so a renamed key fails on a laptop instead of in CI.TestPlaceholderConfigTargetsTheRightKeyAmongSameNamedSiblingspins the exact case the line-prefix match got wrong. Plus comment/blank-line preservation, the unresolvable-path error, block-scalar rejection, and the template renderer including its leftover-token check.Test plan
go build ./...,go vet ./...,gofmtcleango test -short ./...passesscripts/gen-starters.sh(diff -rover both trees, same-version)validate-startersassertions pass against the Go-rendered starters: rejected unedited, rejection comes from the placeholder gate, and it names exactly the expected field paths for both providersmake startersrenders both providers; no leftover__TOKEN__; bothpixi.tomlparse