build(version): stamp deploy metadata - #11
Conversation
|
| Filename | Overview |
|---|---|
| internal/version/version.go | New version helpers replacing a single constant; release() inconsistently omits the unknown guard that shortCommit() applies, and a local variable in commandOutput shadows the package-level function. |
| internal/httpserver/server.go | Extracts health handler, adds commit to startup log, and wires ServiceRevision to observability; the ShortCommit guard discards the computed short hash and logs the raw full-length variable instead. |
| internal/observability/observability.go | Adds ServiceRevision to Options and conditionally appends vcs.ref.head.revision to OTel resource attributes when a real commit is present. |
| internal/observability/sentry/sentry.go | Conditionally attaches vcs.ref.head.revision to Sentry metrics using the same non-empty/non-unknown guard as the OTel path. |
| internal/pkgsite/client.go | User-agent updated to use Public(); warmSearchResult now prefers packagePath over path when extracting the path from a search result item — a correct, unrelated fix bundled in the PR. |
| Dockerfile | Introduces VERSION and COMMIT build args with safe defaults, injected via -ldflags into the binary. |
| .github/workflows/fly-deploy.yml | Passes VERSION=sha-SHORT and full COMMIT=FULL_SHA as build args to flyctl deploy. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["Build time"] -->|version.Version| B["version.Release()"]
A -->|version.Commit| C["version.ShortCommit()"]
B --> D["Startup log"]
B --> E["OTel service.version"]
B --> F["Sentry Release"]
C --> G["OTel vcs.ref.head.revision"]
C --> H["Sentry vcs.ref.head.revision"]
A -->|PublicVersion| I["version.Public()"]
I --> J["MCP Version"]
I --> K["pkgsite User-Agent"]
Reviews (1): Last reviewed commit: "build(version): stamp deploy metadata" | Re-trigger Greptile
| func release(version, commit string) string { | ||
| version = strings.TrimSpace(version) | ||
| if version != "" && version != "dev" { | ||
| return version | ||
| } |
There was a problem hiding this comment.
release() guards against empty and "dev" but not "unknown", while shortCommit() explicitly rejects "unknown". If Version is stamped with "unknown" (e.g. the build arg is unset or the shell fallback fires), release() returns "unknown" verbatim, which would propagate to OTel service.version and Sentry Release without filtering.
| func release(version, commit string) string { | |
| version = strings.TrimSpace(version) | |
| if version != "" && version != "dev" { | |
| return version | |
| } | |
| func release(version, commit string) string { | |
| version = strings.TrimSpace(version) | |
| if version != "" && version != "dev" && version != "unknown" { | |
| return version | |
| } |
| func commandOutput(version, commit string) string { | ||
| release := release(version, commit) | ||
| commit = strings.TrimSpace(commit) |
There was a problem hiding this comment.
The local variable
release shadows the package-level function release() in the same scope. Go resolves the function call on the right-hand side before the variable is declared, so this compiles and runs correctly today, but it makes the code fragile — any future call to release(...) after this line inside commandOutput would silently invoke the string value rather than the function. Renaming the local avoids the hazard.
| func commandOutput(version, commit string) string { | |
| release := release(version, commit) | |
| commit = strings.TrimSpace(commit) | |
| func commandOutput(version, commit string) string { | |
| releaseStr := release(version, commit) | |
| commit = strings.TrimSpace(commit) |
| errCh := make(chan error, 1) | ||
| go func() { | ||
| logger.InfoContext(ctx, "starting pkgsite-mcp http server", slog.String("addr", cfg.Addr), slog.String("version", version.Version)) | ||
| attrs := []any{slog.String("addr", cfg.Addr), slog.String("version", version.Release())} | ||
| if commit := version.ShortCommit(); commit != "" { | ||
| attrs = append(attrs, slog.String("commit", version.Commit)) | ||
| } | ||
| logger.InfoContext(ctx, "starting pkgsite-mcp http server", attrs...) |
There was a problem hiding this comment.
The guard assigns the short commit to
commit but then discards it, logging the raw package-level version.Commit instead. Using ShortCommit() purely as a boolean guard while ignoring the returned value is misleading — either log the short form already in hand, or guard directly on the raw variable.
| errCh := make(chan error, 1) | |
| go func() { | |
| logger.InfoContext(ctx, "starting pkgsite-mcp http server", slog.String("addr", cfg.Addr), slog.String("version", version.Version)) | |
| attrs := []any{slog.String("addr", cfg.Addr), slog.String("version", version.Release())} | |
| if commit := version.ShortCommit(); commit != "" { | |
| attrs = append(attrs, slog.String("commit", version.Commit)) | |
| } | |
| logger.InfoContext(ctx, "starting pkgsite-mcp http server", attrs...) | |
| errCh := make(chan error, 1) | |
| go func() { | |
| attrs := []any{slog.String("addr", cfg.Addr), slog.String("version", version.Release())} | |
| if commit := version.ShortCommit(); commit != "" { | |
| attrs = append(attrs, slog.String("commit", commit)) | |
| } | |
| logger.InfoContext(ctx, "starting pkgsite-mcp http server", attrs...) |
internal/versionhelpers for public, release, commit, and cli version formattingVERSIONandCOMMITthroughDockerfile,Justfile, and fly deploy build args