Conversation
Migrate the entire HTTP layer from the deprecated go-martini framework to
the chi router (github.com/go-chi/chi/v5). This is a framework swap that
preserves all existing URL paths and behavior.
Key changes:
- Replace martini.Classic() with chi.NewRouter() + standard middleware
- Replace martini dependency injection with standard http.HandlerFunc(w, r)
- Replace martini-contrib/render with lightweight renderJSON/renderHTML helpers
- Replace martini-contrib/auth with custom BasicAuth/MultiAuth middleware
- Replace martini-contrib/gzip with chi middleware.Compress
- Replace martini URL params (:param) with chi params ({param})
- Replace params["x"] with chi.URLParam(r, "x")
- Replace martini's raft reverse proxy handler with chi middleware pattern
- Update ssl.VerifyOUs to return standard http middleware instead of martini.Handler
- Remove go-martini, martini-contrib/*, and codegangsta/inject dependencies
Closes #35
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (91)
📝 WalkthroughWalkthroughThis pull request migrates the HTTP layer from the unmaintained Martini framework to Chi v5, a lightweight, stdlib-compatible router. The migration includes replacing Martini route handlers, middleware, and dependency injection with Chi-style handlers, new authentication and rendering middlewares, and updated request parameter extraction patterns throughout the HTTP API and web UI handlers. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Middleware as Chi Middleware Stack
participant Router as Chi Router/Mux
participant Handler as HTTP Handler
participant RenderHelper as Render Helpers
Client->>Middleware: HTTP Request
Middleware->>Middleware: RequestID, Logger, Recoverer, Auth
Middleware->>Router: Pass Request (authenticated)
Router->>Router: Match Pattern & Extract URLParams
Router->>Handler: Route to Matched Handler
Handler->>Handler: Extract params via chi.URLParam()
Handler->>Handler: Process Business Logic
Handler->>RenderHelper: renderJSON(w, status, data)
RenderHelper->>Handler: JSON Encoded Response
Handler->>Client: HTTP Response with Status & Body
Client-->>Middleware: Response Complete
Middleware->>Client: Log Response (status, duration)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on modernizing the orchestrator's HTTP routing and middleware handling by replacing the outdated Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Pull request overview
This PR migrates the project’s HTTP layer from the deprecated go-martini framework to the chi router, replacing Martini DI-style handlers and middleware with standard net/http handlers and chi middleware while removing martini-related dependencies.
Changes:
- Replace martini routing/middleware with
github.com/go-chi/chi/v5for API, web UI, and agent endpoints. - Introduce new auth, raft reverse-proxy, TLS OU-verification middleware, and lightweight JSON/HTML/redirect render helpers.
- Update vendored dependencies to remove martini-related packages and add chi.
Reviewed changes
Copilot reviewed 9 out of 92 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| go/app/http.go | Switch server setup to chi router and chi middleware; update auth/static/TLS middleware wiring. |
| go/http/httpbase.go | Replace martini auth user injection with request-context user storage; add Basic/Multi auth middleware and chi param helpers. |
| go/http/render.go | Add new JSON/HTML/redirect rendering helpers and template caching. |
| go/http/raft_reverse_proxy.go | Convert raft leader proxying from martini handler to standard middleware. |
| go/ssl/ssl.go | Convert OU verification from martini handler to standard middleware. |
| go/http/agents_api.go | Migrate agents API handlers from martini DI/render to http.ResponseWriter + *http.Request + chi params. |
| go/http/api_test.go | Update tests to use chi router registration. |
| go.mod | Remove martini-related deps; add github.com/go-chi/chi/v5. |
| go.sum | Update sums to reflect dependency changes. |
| vendor/modules.txt | Vendor manifest updates (remove martini deps, add chi). |
| vendor/... | Vendored dependency tree updated (remove martini/auth/gzip/render/inject/bpool, add chi + middleware, and associated files). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func (this *HttpAgentsAPI) SetHostAttribute(params martini.Params, r render.Render, req *http.Request) { | ||
| err := attributes.SetHostAttributes(params["host"], params["attrVame"], params["attrValue"]) | ||
| func (this *HttpAgentsAPI) SetHostAttribute(w http.ResponseWriter, r *http.Request) { | ||
| err := attributes.SetHostAttributes(chi.URLParam(r, "host"), chi.URLParam(r, "attrVame"), chi.URLParam(r, "attrValue")) |
There was a problem hiding this comment.
The URL param placeholder/key attrVame looks like a typo (should likely be attrName). With chi, the placeholder name is internal-only and can be corrected without changing the external URL path structure. Renaming it would reduce confusion and prevent propagating the typo into new code.
| err := attributes.SetHostAttributes(chi.URLParam(r, "host"), chi.URLParam(r, "attrVame"), chi.URLParam(r, "attrValue")) | |
| err := attributes.SetHostAttributes(chi.URLParam(r, "host"), chi.URLParam(r, "attrName"), chi.URLParam(r, "attrValue")) |
| m := martini.Classic() | ||
| router := chi.NewRouter() | ||
|
|
||
| // Middleware |
There was a problem hiding this comment.
The new middleware stack uses chi/middleware.Logger, which logs r.RemoteAddr directly. Previously martini's logger preferred X-Real-IP/X-Forwarded-For, so deployments behind a reverse proxy may lose real client IPs in logs. If you still expect the old behavior, add chi/middleware.RealIP early (or implement an equivalent proxy-aware logger) and document the trust assumptions for these headers.
| // Middleware | |
| // Middleware | |
| router.Use(chimiddleware.RealIP) |
| layoutPath := filepath.Join(templateDir, layoutFile+".tmpl") | ||
| tmplPath := filepath.Join(templateDir, name+".tmpl") | ||
| t, err := template.ParseFiles(layoutPath, tmplPath) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
layout.tmpl uses the {{yield}} helper, but getTemplate() parses templates without registering yield (or current) funcs. This will cause template parsing to fail and the web UI to return 500 for every page. Add the helper funcs to the template FuncMap before parsing (and ensure the per-request yield implementation renders the requested page template).
| w.Header().Set("Content-Type", "text/html; charset=UTF-8") | ||
| w.WriteHeader(status) | ||
| if err := t.Execute(w, data); err != nil { | ||
| log.Errorf("Error executing template %s: %+v", name, err) | ||
| } |
There was a problem hiding this comment.
renderHTML writes headers/status before executing the template. If template execution fails mid-stream, the response may be partially written and cannot be converted into a clean 500. Consider rendering into a buffer first (as the old martini renderer did) and only writing headers/body after successful execution.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a significant and well-executed migration from the deprecated go-martini framework to chi. The changes are extensive, touching the entire HTTP layer, but appear to be handled correctly by replacing dependency injection with standard http.HandlerFunc signatures and context-based value passing. The introduction of custom middleware for authentication and helper functions for rendering standardizes the new approach. I've found a couple of issues: one critical bug that would cause a runtime panic, and one area for performance improvement in template caching. Overall, this is a great improvement that modernizes the codebase and removes unmaintained dependencies.
|
|
||
| // go-metrics | ||
| m.Get(this.URLPrefix+"/debug/metrics", exp.ExpHandler(metrics.DefaultRegistry)) | ||
| router.Get(this.URLPrefix+"/debug/metrics", exp.ExpHandler(metrics.DefaultRegistry).(http.HandlerFunc)) |
There was a problem hiding this comment.
This line will cause a runtime panic. exp.ExpHandler returns an http.Handler, but not an http.HandlerFunc. The type assertion .(http.HandlerFunc) will fail. You should use router.Handle which accepts an http.Handler.
| router.Get(this.URLPrefix+"/debug/metrics", exp.ExpHandler(metrics.DefaultRegistry).(http.HandlerFunc)) | |
| router.Handle(this.URLPrefix+"/debug/metrics", exp.ExpHandler(metrics.DefaultRegistry)) |
| func getTemplate(name string) (*template.Template, error) { | ||
| templateCache.RLock() | ||
| if t, ok := templateCache.m[name]; ok { | ||
| templateCache.RUnlock() | ||
| return t, nil | ||
| } | ||
| templateCache.RUnlock() | ||
|
|
||
| layoutPath := filepath.Join(templateDir, layoutFile+".tmpl") | ||
| tmplPath := filepath.Join(templateDir, name+".tmpl") | ||
| t, err := template.ParseFiles(layoutPath, tmplPath) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| templateCache.Lock() | ||
| templateCache.m[name] = t | ||
| templateCache.Unlock() | ||
| return t, nil | ||
| } |
There was a problem hiding this comment.
The current implementation of getTemplate has a race condition that can lead to inefficient template parsing. If multiple goroutines request the same uncached template simultaneously, they will all parse the template file, with only the last one's result being stored. This can be optimized to avoid redundant work.
func getTemplate(name string) (*template.Template, error) {
templateCache.RLock()
t, ok := templateCache.m[name]
templateCache.RUnlock()
if ok {
return t, nil
}
templateCache.Lock()
defer templateCache.Unlock()
// Re-check for the template, as another goroutine may have added it
// while we were waiting for the lock.
if t, ok := templateCache.m[name]; ok {
return t, nil
}
layoutPath := filepath.Join(templateDir, layoutFile+".tmpl")
tmplPath := filepath.Join(templateDir, name+".tmpl")
t, err := template.ParseFiles(layoutPath, tmplPath)
if err != nil {
return nil, err
}
templateCache.m[name] = t
return t, nil
}
Summary
github.com/go-chi/chi/v5)http.HandlerFunc(w, r)signatures across 100+ API routes, web routes, and agent routesDetails
This is a framework swap that preserves all existing URL paths and API behavior:
m.Get("/api/path", handler)replaced withr.Get("/api/path", handler)using chi:paramstyle replaced with chi{param}styleparams martini.Params, r render.Render, req *http.Request, user auth.Userreplaced with standardw http.ResponseWriter, r *http.RequestBasicAuthMiddlewareandMultiAuthMiddlewarereplace martini-contrib/auth, storing user in request contextrenderJSON/renderHTML/renderRedirecthelpers replace martini-contrib/rendermiddleware.Compressreplaces martini-contrib/gzipVerifyOUsconverted frommartini.Handlerto standardfunc(http.Handler) http.HandlermiddlewareCloses #35
Test plan
go buildsucceedsgo test ./go/http/...passes (all 7 tests)go test -vet=off ./go/...passes (pre-existing vet issues in other packages unrelated to this change)Summary by CodeRabbit