build(swagger): compile swagger UI only for dev builds#280
Conversation
📝 WalkthroughWalkthroughConditional Swagger support was added using Go build tags plus a runtime Changes
Sequence DiagramsequenceDiagram
participant Build as Build System
participant Main as cmd/gomodel
participant Config as Config Loader
participant App as Application Init
participant Server as HTTP Server
participant Swagger as Swagger Registrar
Build->>Main: Compile with or without -tags=swagger
Main->>Config: Load configuration (server.swagger_enabled)
Main->>Main: Call configureSwaggerDocs(basePath) [build-tagged]
Main->>App: Initialize application
App->>Server: Initialize server
Server->>Server: Call SwaggerAvailable() [build-tagged]
Server->>App: Return availability
App->>Server: Use effective swagger_enabled = config && availability
alt Swagger available and enabled
Server->>Swagger: registerSwagger(e, cfg)
Swagger-->>Server: Handler wired (echo-swagger)
else Swagger unavailable or disabled
Server->>Swagger: registerSwagger(e, cfg) -> no-op
end
Server-->>App: Server ready
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR gates Swagger UI compilation and registration behind a The refactor is clean and consistent: build-tag stub/implementation pairs are added at both the Confidence Score: 5/5Safe to merge — clean conditional-compilation refactor with no logic regressions All findings are P2 or lower. The build-tag split is correct and symmetric at both the cmd and server layers. The default-false change is intentional and tested. The JS rename cleanly excludes test fixtures from the embed glob. No security, data, or correctness issues identified. No files require special attention Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A{Built with -tags=swagger?} -->|Yes| B[SwaggerAvailable returns true]
A -->|No| C[SwaggerAvailable returns false]
D{config swagger_enabled=true?} -->|Yes| E{SwaggerAvailable?}
D -->|No| F[swaggerEnabled = false]
E -->|Yes| G[swaggerEnabled = true]
E -->|No| H[swaggerEnabled = false + slog.Warn emitted]
B --> E
C --> E
G --> I[registerSwagger registers route]
F --> J[registerSwagger is no-op]
H --> J
Reviews (1): Last reviewed commit: "build(swagger): compile UI only for dev ..." | Re-trigger Greptile |
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/server/swagger_disabled_test.go`:
- Around line 11-23: Add a fast-fail assertion in
TestSwaggerEndpoint_RequestedButNotAvailable to explicitly verify the build
constraint by calling SwaggerAvailable() and asserting it returns false before
making the HTTP request; this ensures if swagger_disabled.go gets inverted the
test fails immediately. Update the test
(TestSwaggerEndpoint_RequestedButNotAvailable) to call SwaggerAvailable() and
assert equality to false, keeping the existing Config{SwaggerEnabled: true}
setup and existing HTTP request/assertion logic intact.
In `@Makefile`:
- Line 25: The Makefile currently hard-sets LOG_LEVEL and SWAGGER_ENABLED inline
in the run recipe, preventing caller overrides; change those to default variable
assignments (use LOG_LEVEL?=debug and SWAGGER_ENABLED?=true) and update the run
target to use the variables (e.g., invoke go run with LOG_LEVEL=$(LOG_LEVEL) and
SWAGGER_ENABLED=$(SWAGGER_ENABLED)) while keeping the -tags=swagger and
./cmd/gomodel invocation so callers like LOG_LEVEL=info make run can override
defaults.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 49205e04-5723-448b-ac49-70fb895f6328
📒 Files selected for processing (29)
Makefilecmd/gomodel/main.gocmd/gomodel/swagger_disabled.gocmd/gomodel/swagger_enabled.goconfig/config.example.yamlconfig/config.goconfig/config_test.gointernal/admin/dashboard/static/js/modules/aliases.test.cjsinternal/admin/dashboard/static/js/modules/audit-list.test.cjsinternal/admin/dashboard/static/js/modules/auth-keys.test.cjsinternal/admin/dashboard/static/js/modules/charts.test.cjsinternal/admin/dashboard/static/js/modules/dashboard-display.test.cjsinternal/admin/dashboard/static/js/modules/dashboard-layout.test.cjsinternal/admin/dashboard/static/js/modules/dashboard-paths.test.cjsinternal/admin/dashboard/static/js/modules/guardrails.test.cjsinternal/admin/dashboard/static/js/modules/providers.test.cjsinternal/admin/dashboard/static/js/modules/request-cancellation.test.cjsinternal/admin/dashboard/static/js/modules/timestamp-format.test.cjsinternal/admin/dashboard/static/js/modules/timezone-layout.test.cjsinternal/admin/dashboard/static/js/modules/timezone.test.cjsinternal/admin/dashboard/static/js/modules/workflows-layout.test.cjsinternal/admin/dashboard/static/js/modules/workflows.test.cjsinternal/app/app.gointernal/server/http.gointernal/server/http_test.gointernal/server/swagger_disabled.gointernal/server/swagger_disabled_test.gointernal/server/swagger_enabled.gointernal/server/swagger_enabled_test.go
💤 Files with no reviewable changes (1)
- internal/server/http_test.go
| func TestSwaggerEndpoint_RequestedButNotAvailable(t *testing.T) { | ||
| mock := &mockProvider{} | ||
| srv := New(mock, &Config{SwaggerEnabled: true}) | ||
|
|
||
| req := httptest.NewRequest(http.MethodGet, "/swagger/index.html", nil) | ||
| rec := httptest.NewRecorder() | ||
|
|
||
| srv.ServeHTTP(rec, req) | ||
|
|
||
| if rec.Code != http.StatusNotFound { | ||
| t.Errorf("expected status 404, got %d", rec.Code) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Good behavioral coverage of the !swagger build.
Asserting 404 against a Config{SwaggerEnabled: true} proves the route is absent regardless of config when the binary is built without -tags=swagger. Optionally consider also asserting SwaggerAvailable() == false here to fail fast if someone accidentally inverts the build constraints in swagger_disabled.go.
♻️ Optional belt-and-suspenders assertion
func TestSwaggerEndpoint_RequestedButNotAvailable(t *testing.T) {
+ if SwaggerAvailable() {
+ t.Fatal("SwaggerAvailable() must be false in !swagger build")
+ }
mock := &mockProvider{}
srv := New(mock, &Config{SwaggerEnabled: true})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/server/swagger_disabled_test.go` around lines 11 - 23, Add a
fast-fail assertion in TestSwaggerEndpoint_RequestedButNotAvailable to
explicitly verify the build constraint by calling SwaggerAvailable() and
asserting it returns false before making the HTTP request; this ensures if
swagger_disabled.go gets inverted the test fails immediately. Update the test
(TestSwaggerEndpoint_RequestedButNotAvailable) to call SwaggerAvailable() and
assert equality to false, keeping the existing Config{SwaggerEnabled: true}
setup and existing HTTP request/assertion logic intact.
SantiagoDePolonia
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Makefile (1)
111-112:⚠️ Potential issue | 🟡 Minor
linttarget is missing theswaggerbuild tag — swagger-tagged files won't be linted.
--build-tags=e2e,integration,contractdoes not includeswagger, so the newcmd/gomodel/swagger_enabled.goandinternal/server/swagger_enabled.go(both gated by//go:build swagger) are invisible togolangci-lint. Their counterparts (*_disabled.go) are linted, but the swagger-on path silently bypasses all linters, including theecho-swaggerimport path. Addswaggerto the tag list to keep both build variants under lint coverage:🔧 Proposed fix
lint: - golangci-lint run --build-tags=e2e,integration,contract ./cmd/... ./config/... ./internal/... ./tests/... + golangci-lint run --build-tags=e2e,integration,contract,swagger ./cmd/... ./config/... ./internal/... ./tests/...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 111 - 112, The lint target currently runs golangci-lint with --build-tags=e2e,integration,contract which excludes swagger-tagged files; update the Makefile's lint target (the "lint" rule) to include the swagger build tag so files like cmd/gomodel/swagger_enabled.go and internal/server/swagger_enabled.go (//go:build swagger) are included in linting, ensuring imports such as echo-swagger are checked too.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Makefile`:
- Line 55: The Makefile currently invokes node with an explicit glob
"internal/admin/dashboard/static/js/modules/*.test.cjs" which the shell may pass
literally when no matches exist; update the test recipe to avoid shell glob
expansion by either (A) using a guarded finder that only invokes node --test
when matching files are present (use find to locate "*.test.cjs" under the
internal/admin/dashboard/static/js/modules directory and call node --test with
those files), or (B) remove the explicit glob and rely on Node's test discovery
by calling node --test against the modules directory (e.g., node --test
internal/admin/dashboard/static/js/modules) so Node discovers tests itself;
ensure the chosen approach prevents a failing "Cannot find module" when no
.test.cjs files exist.
- Line 27: The Makefile's run recipe hard-codes the swagger build tag in the go
run invocation (the token "go run -tags=swagger ./cmd/gomodel"), preventing
callers from opting out; change the recipe to use a parameterized build-tags
variable (e.g., TAGS or BUILD_TAGS) with a sensible default (swagger) and only
append the "-tags=$(TAGS)" flag when the variable is non-empty so callers can do
TAGS= make run or TAGS="" make run to run a non-swagger binary; update the run
target to reference this variable instead of the literal "-tags=swagger".
---
Outside diff comments:
In `@Makefile`:
- Around line 111-112: The lint target currently runs golangci-lint with
--build-tags=e2e,integration,contract which excludes swagger-tagged files;
update the Makefile's lint target (the "lint" rule) to include the swagger build
tag so files like cmd/gomodel/swagger_enabled.go and
internal/server/swagger_enabled.go (//go:build swagger) are included in linting,
ensuring imports such as echo-swagger are checked too.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| # Run the application | ||
| run: | ||
| go run ./cmd/gomodel | ||
| LOG_LEVEL=$(LOG_LEVEL) SWAGGER_ENABLED=$(SWAGGER_ENABLED) go run -tags=swagger ./cmd/gomodel |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
make run always forces -tags=swagger, even when caller overrides SWAGGER_ENABLED=false.
The build tag is hard-coded on the recipe line, so callers can disable runtime Swagger via SWAGGER_ENABLED=false make run but cannot opt out of the swagger-tagged binary (which carries the echo-swagger and swaggerdocs deps). For symmetry with the new override-friendly env vars, consider also parameterizing the build tags so a dev can run a prod-shaped binary locally:
♻️ Optional: parameterize build tags too
+GO_TAGS ?= swagger
...
run:
- LOG_LEVEL=$(LOG_LEVEL) SWAGGER_ENABLED=$(SWAGGER_ENABLED) go run -tags=swagger ./cmd/gomodel
+ LOG_LEVEL=$(LOG_LEVEL) SWAGGER_ENABLED=$(SWAGGER_ENABLED) go run $(if $(GO_TAGS),-tags=$(GO_TAGS)) ./cmd/gomodel🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Makefile` at line 27, The Makefile's run recipe hard-codes the swagger build
tag in the go run invocation (the token "go run -tags=swagger ./cmd/gomodel"),
preventing callers from opting out; change the recipe to use a parameterized
build-tags variable (e.g., TAGS or BUILD_TAGS) with a sensible default (swagger)
and only append the "-tags=$(TAGS)" flag when the variable is non-empty so
callers can do TAGS= make run or TAGS="" make run to run a non-swagger binary;
update the run target to reference this variable instead of the literal
"-tags=swagger".
| # Run dashboard JavaScript unit tests | ||
| test-dashboard: | ||
| node --test internal/admin/dashboard/static/js/modules/*.test.js | ||
| node --test internal/admin/dashboard/static/js/modules/*.test.cjs |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Glob expansion happens in the shell, not in node --test.
internal/admin/dashboard/static/js/modules/*.test.cjs is expanded by the shell at recipe time. If no .cjs test files exist yet (e.g., during the rename transition), the literal pattern is passed to node --test, which will fail with an unhelpful "Cannot find module" error rather than a clean "no tests" exit. With shell-glob nullglob not set in make's default /bin/sh, this is a footgun; consider either guarding with find or relying on Node's built-in test discovery:
♻️ More robust forms
- node --test internal/admin/dashboard/static/js/modules/*.test.cjs
+ node --test --test-name-pattern='.*' \
+ $$(find internal/admin/dashboard/static/js/modules -name '*.test.cjs')Or, if the directory only contains tests of a single style, let Node discover them:
- node --test internal/admin/dashboard/static/js/modules/*.test.cjs
+ node --test internal/admin/dashboard/static/js/modules📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| node --test internal/admin/dashboard/static/js/modules/*.test.cjs | |
| node --test --test-name-pattern='.*' \ | |
| $$(find internal/admin/dashboard/static/js/modules -name '*.test.cjs') |
| node --test internal/admin/dashboard/static/js/modules/*.test.cjs | |
| node --test internal/admin/dashboard/static/js/modules |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Makefile` at line 55, The Makefile currently invokes node with an explicit
glob "internal/admin/dashboard/static/js/modules/*.test.cjs" which the shell may
pass literally when no matches exist; update the test recipe to avoid shell glob
expansion by either (A) using a guarded finder that only invokes node --test
when matching files are present (use find to locate "*.test.cjs" under the
internal/admin/dashboard/static/js/modules directory and call node --test with
those files), or (B) remove the explicit glob and rely on Node's test discovery
by calling node --test against the modules directory (e.g., node --test
internal/admin/dashboard/static/js/modules) so Node discovers tests itself;
ensure the chosen approach prevents a failing "Cannot find module" when no
.test.cjs files exist.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Makefile`:
- Line 112: The Makefile's lint-fix target is out of sync with lint: update the
lint-fix target (and optionally factor into variables) so it uses the same
--build-tags=swagger,e2e,integration,contract and the same path list (./cmd/...
./config/... ./internal/... ./tests/...) as the lint target; modify the lint-fix
recipe to invoke golangci-lint run --build-tags=swagger,e2e,integration,contract
against the full path set (or extract BUILD_TAGS and LINT_PATHS variables and
reference them in both lint and lint-fix) so files behind the
swagger/e2e/integration/contract tags are analyzed and auto-fixed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| # Run linter | ||
| lint: | ||
| golangci-lint run --build-tags=e2e,integration,contract ./cmd/... ./config/... ./internal/... ./tests/... | ||
| golangci-lint run --build-tags=swagger,e2e,integration,contract ./cmd/... ./config/... ./internal/... ./tests/... |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
lint-fix is out of sync with lint build tags.
The lint target now passes --build-tags=swagger,e2e,integration,contract, but lint-fix (line 116) still runs without any build tags and against a narrower path set (./cmd/... ./config/... ./internal/..., omitting ./tests/...). As a result, files behind those tags (e.g., cmd/gomodel/swagger_enabled.go with //go:build swagger, plus e2e/integration/contract test files) won't be analyzed or auto-fixed, so make lint-fix followed by make lint can still surface findings the fixer never saw.
♻️ Proposed alignment
# Run linter with auto-fix
lint-fix:
- golangci-lint run --fix ./cmd/... ./config/... ./internal/...
+ golangci-lint run --fix --build-tags=swagger,e2e,integration,contract ./cmd/... ./config/... ./internal/... ./tests/...Optionally, factor the tag list and path list into Make variables to keep the two targets in lockstep going forward.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| golangci-lint run --build-tags=swagger,e2e,integration,contract ./cmd/... ./config/... ./internal/... ./tests/... | |
| # Run linter with auto-fix | |
| lint-fix: | |
| golangci-lint run --fix --build-tags=swagger,e2e,integration,contract ./cmd/... ./config/... ./internal/... ./tests/... |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Makefile` at line 112, The Makefile's lint-fix target is out of sync with
lint: update the lint-fix target (and optionally factor into variables) so it
uses the same --build-tags=swagger,e2e,integration,contract and the same path
list (./cmd/... ./config/... ./internal/... ./tests/...) as the lint target;
modify the lint-fix recipe to invoke golangci-lint run
--build-tags=swagger,e2e,integration,contract against the full path set (or
extract BUILD_TAGS and LINT_PATHS variables and reference them in both lint and
lint-fix) so files behind the swagger/e2e/integration/contract tags are analyzed
and auto-fixed.
2 participants
Summary
.test.cjsso production*.jsembeds exclude test fixtures while keeping the embed glob simpleswaggerbuild tagmake runopt into dev defaults withLOG_LEVEL=debug,SWAGGER_ENABLED=true, and-tags=swaggerswagger_enabledto false and warn if a non-swagger build is configured to enable itVerification
go test ./cmd/... ./internal/... ./config/...go test -tags=swagger ./cmd/gomodel ./internal/servergo build -trimpath -ldflags="-s -w" ./cmd/gomodelgo build -tags=swagger -trimpath -ldflags="-s -w" ./cmd/gomodelSize check
Summary by CodeRabbit
New Features
Changes
Chores
Tests