Make g10k's core functionality an importable api - #271
Merged
dhollinger merged 16 commits intoSep 19, 2026
Merged
Conversation
dhollinger
force-pushed
the
refactor/pkg-g10k-api-phase2
branch
2 times, most recently
from
September 9, 2026 23:36
73afc32 to
b8f15a7
Compare
dhollinger
requested review from
TheMeier and
xorpaul
and
a lite review from Copilot
September 9, 2026 23:40
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
It introduces a breaking CLI flag change (-version -> -versions) and the golangci-lint version differs between CI and the Makefile, risking inconsistent lint results.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR completes the separation of the g10k CLI from the core implementation so the project can be imported and used as a Go API (core code under pkg/g10k, CLI under cmd/g10k), while also updating build/test tooling for the new layout and enforcing timeouts for git/Forge operations.
Changes:
- Refactors the core from
package mainintopackage g10kunderpkg/g10k, and introduces a dedicated CLI entrypoint atcmd/g10k/main.go. - Updates tests and test fixtures to work from the new package working directory (path adjustments, git maintenance/gc disabled in tests, purge tests isolated from leftover cache).
- Updates build/release/CI configuration for the new layout (Makefile targets, GoReleaser main path, module path change, Go toolchain bump, Docker builder image).
File summaries
| File | Description |
|---|---|
| tests/TestConfigUseSSHAgent.yaml | Adjusts fixture paths for private key under new test working directory. |
| tests/TestConfigPrivateGithub.yaml | Normalizes quoting and updates SSH key fixture path. |
| tests/TestConfigPostrunCommandDirs.yaml | Normalizes quoting and updates postrun script path. |
| tests/TestConfigPostrunCommand.yaml | Normalizes quoting and updates postrun script path. |
| tests/TestConfigFullworkingBranchFilter.yaml | Normalizes quoting and updates filter_command path. |
| pkg/g10k/stale.go | Moves file into importable package (package g10k). |
| pkg/g10k/puppetfile.go | Moves file into importable package (package g10k). |
| pkg/g10k/modules.go | Moves file into importable package (package g10k). |
| pkg/g10k/helper.go | Adds command execution timeout enforcement via exec.CommandContext. |
| pkg/g10k/helper_test.go | Adds tests for command timeout; disables git maintenance/gc in test runs. |
| pkg/g10k/git.go | Moves file into importable package (package g10k). |
| pkg/g10k/g10k.go | Removes embedded CLI main; sets default timeout in pfMode config. |
| pkg/g10k/g10k_test.go | Updates test paths/fixtures for new pkg layout; improves purge test isolation and failure output. |
| pkg/g10k/g10k_puppetfile_test.go | Updates puppetfile tests to construct a Runtime per subprocess and fixes fixture paths. |
| pkg/g10k/forge.go | Enforces configured timeout on Forge HTTP operations. |
| pkg/g10k/config.go | Moves file into importable package (package g10k). |
| Makefile | Updates build/test/lint/vet/imports/clean targets for pkg/ + cmd/ layout and env overrides. |
| go.mod | Renames module path to github.com/voxpupuli/g10k and bumps Go version. |
| Dockerfile | Updates builder image/toolchain and build invocation for new layout. |
| cmd/g10k/main.go | New CLI entrypoint that wires flags into pkg/g10k.Run. |
| .goreleaser.yaml | Points GoReleaser build main to ./cmd/g10k. |
| .gitignore | Ignores only the root g10k binary. |
| .github/workflows/ci.yml | Updates CI Go version and linter version; keeps GoReleaser snapshot job aligned to new layout. |
Review details
- Files reviewed: 22/23 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Move flag parsing and the version banner out of the library package. pkg/g10k now only exposes the Options/Runtime API and Run(), the g10k binary is built from cmd/g10k.
The move into pkg/ left the Makefile with a root g10k.go prerequisite that no longer exists, breaking make vet/imports/build/g10k and with that `make test` in CI. Point the build targets at ./cmd/g10k, vet and run goimports over the actual source dirs, and build ./cmd/g10k in goreleaser instead of the packageless repo root. NOTE: This step utilized AI to help speed up the process. All generated code was reviewed by a human.
git >= 2.55 spawns a detached `git maintenance run --auto --detach` (commit-graph write) after fetches. Those processes outlive the test subprocess and keep writing into the module cache after the test purged it, leaving a partial non-git directory behind. The next test's resolve then fatals in doMirrorOrUpdate() with `Failed to clone or pull`, which made TestPurgeStaleDeploymentOnly fail whenever it ran after TestPurgeStalePuppetfileOnly. Set maintenance.auto=false and gc.auto=0 via GIT_CONFIG_* env vars in TestMain so every process in the test tree, including the spawned subprocesses and their git children, inherits them. NOTE: This step utilized AI to help speed up the process. All generated code was reviewed by a human.
Pre-clean /tmp/g10k and /tmp/full in the parent before spawning the resolve subprocess, so each TestPurgeStale* test starts from a deterministic state no matter what earlier tests left behind. Also print the subprocess output when TestPurgeStaleDeploymentOnly exits with an unexpected exit code to make future failures diagnosable. NOTE: This step utilized AI to help speed up the process. All generated code was reviewed by a human.
checkExitCodeAndOutputOfReadPuppetfileSubprocess() now receives the Runtime instead of constructing its own, so each delegating test builds its Runtime via NewRuntime() like the rest of the suite. NOTE: This step utilized AI to help speed up the process. All generated code was reviewed by a human.
The lint targets passed an undefined $(pkgs) variable, which only worked by accident because golangci-lint defaults to ./... when no paths are given. Be explicit now that the sources live in pkg/g10k and cmd/g10k. The clean target still removed the ./cache and ./example test artifacts from the repo root, but the tests create them in their working directory pkg/g10k since the move. NOTE: This step utilized AI to help speed up the process. All generated code was reviewed by a human.
The image still built on golang:1.17-alpine while go.mod requires Go >= 1.25.3, which broke the container build and with that `make build-image`. Pin the builder to the same Go version the CI uses; the image builds and the g10k binary runs again. NOTE: This step utilized AI to help speed up the process. All generated code was reviewed by a human.
The timeout config setting (default: 5) was documented as "Timeout in seconds for git and forge operations" but never enforced: executeCommand() accepted the value and ignored it, and the Forge HTTP clients were built without a Timeout. Any stalled git-over-SSH connection or Forge response therefore blocked a resolve indefinitely, which made test runs hang until the 10 minute go test timeout killed them. Kill commands via exec.CommandContext() when a timeout is set, keeping the previous unbounded behavior for a zero timeout, and add a WaitDelay so killed commands do not block on pipes held by orphaned child processes. Set the same timeout on all Forge HTTP clients and give the puppetfile-mode config the documented default of 5 seconds. NOTE: This step utilized AI to help speed up the process. All generated code was reviewed by a human.
Use conditional assignment for FIRST_GOPATH, SKIP_GOLANGCI_LINT and GOLANGCI_LINT so they can be set from the environment, e.g. exporting SKIP_GOLANGCI_LINT=1 in a shell instead of passing it on every make invocation.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
dhollinger
force-pushed
the
refactor/pkg-g10k-api-phase2
branch
from
September 16, 2026 12:50
f5596cb to
44a3dde
Compare
TheMeier
approved these changes
Sep 17, 2026
TheMeier
left a comment
TheMeier
left a comment
Contributor
There was a problem hiding this comment.
As mentioned. Not happy with so many different aspects in a single PR. The timeout fix has the potential to require some users to change configs.
Apart from that LGTM
Member
Author
|
@TheMeier I don't particularly like it either, however, all these changes would have to go in before another release anyway, so........ Leaving any of these out puts the codebase in an unreleaseable state..... either due to broken release tasks, broken tests, or cli binary that can't be compiled. |
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.
Overview
The goal of this PR is to finish the process of separating out the
g10kcommand line tool from the functionality. This allows the module to be imported as an api for use in other tools without requiring the command line tool. This should also make it slightly easier to add new functionality iteratively tog10kas a whole.To complete this process, the tests were updated to account for the moved files and the Makefile has an update that will allow local environment variables override those set in the Makefile. This proved a problem when trying to run tests locally with a different GOPATH than was expected.