Conversation
re-implement the Puppetfile parser using goyacc.
All current tests pass. Added some new tests. Skipped test (as before)
=== RUN TestPrivateGithubRepository
g10k_test.go:2731: Skipping TestPrivateGithubRepository test, because the test SSH key 'tests/github-test-private/github-test-private' is missing
As with the previous implementation, the parser is not correct for all
possible Puppetfiles, since the Puppetfile syntax is ruby code. However,
it should behave the same as before.
This should allow to improve in the future, eg missing support for git
module defs with digest in the version field.
Optimally this parser and the Pupptefile type would go into a separate
package, but I leave that for a future refactoring.
Co-authored-by: Copilot <copilot@github.com>
| generate-parser: | ||
| go generate ./... |
There was a problem hiding this comment.
Should this be:
| generate-parser: | |
| go generate ./... | |
| puppetfile_parser.go: puppetfile_parser.y | |
| go generate ./... |
If you then add puppetfile_parser.go as a dependency to all the steps that actually need it you can keep keep puppetfile_parser.go in .gitignore.
There was a problem hiding this comment.
valid point. As far as I have seen so far it is quite common in the go-world to commit generated code (eg from protobuf, yacc, openapi) and only re-generate it in CI to check for diversions. But I might be wrong here.
There was a problem hiding this comment.
IMHO you don't want to check in generated code in git and also make it easy to regenerate files if the source changes.
Perhaps https://github.com/voxpupuli/g10k#building should describe this setup?
There was a problem hiding this comment.
As I said in go that is actuall very common (even prefered),
Claude:
The context is clear: the PR adds a goyacc-based parser, generating puppetfile_parser.go from puppetfile_parser.y.
Best Practice: Commit generated files in this case
The Go ecosystem has a well-established consensus here:
✅ Commit generated files when…
- They are needed to build the project without extra tooling installed (
goyaccis not a standard Go tool) - They are part of the public source (not a build artifact like a binary)
- The generator (
goyacc) has versioning concerns — regenerating with a different version may produce subtle differences go generateis used as the convention — the file is checked in, the//go:generatedirective documents how to regenerate it
❌ Don't commit generated files when…
- They are large binaries/executables
- They are trivially reproducible in a standard build step (e.g.,
go buildoutput) - They change frequently and create noisy diffs
Go community precedent
The Go standard library itself commits goyacc-generated files (e.g., go/parser). Projects like golang.org/x/tools and many others follow the same pattern with //go:generate comments.
Recommendation for this PR:
- ✅ Commit
puppetfile_parser.go(generated) - Add a
//go:generate goyacc -o puppetfile_parser.go puppetfile_parser.ycomment at the top of the file or in a related.gofile - ✅ Also commit
y.outputonly if it aids debugging — otherwise add it to.gitignore
This makes the repo self-contained: go build just works, no extra tooling required.
Gemini:
The short answer is: it depends, but the Go community heavily leans toward yes, you should check them in.
While standard Git practice in other languages (like Java or C#) is to never commit generated code, Go has a unique philosophy around dependency management, reproducibility, and simplicity that flips this rule on its head.
re-implement the Puppetfile parser using goyacc.
All current tests pass. Added some new tests. Skipped test (as before)
=== RUN TestPrivateGithubRepository
g10k_test.go:2731: Skipping TestPrivateGithubRepository test, because the test SSH key 'tests/github-test-private/github-test-private' is missing
As with the previous implementation, the parser is not correct for all possible Puppetfiles, since the Puppetfile syntax is ruby code. However, it should behave the same as before.
This should allow to improve in the future, eg missing support for git module defs with digest in the version field.
Optimally this parser and the Pupptefile type would go into a separate package, but I leave that for a future refactoring.