Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pass along version to httpd/handler #2928

Merged
merged 1 commit into from
Jun 12, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions cmd/influxd/run/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,7 @@ func (s *Server) appendHTTPDService(c httpd.Config) {
srv.Handler.MetaStore = s.MetaStore
srv.Handler.QueryExecutor = s.QueryExecutor
srv.Handler.PointsWriter = s.PointsWriter
srv.Handler.Version = s.Version

// If a ContinuousQuerier service has been started, attach it.
for _, srvc := range s.Services {
Expand Down
11 changes: 5 additions & 6 deletions services/httpd/handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ type route struct {
type Handler struct {
mux *pat.PatternServeMux
requireAuthentication bool
version string
Version string

MetaStore interface {
Database(name string) (*meta.DatabaseInfo, error)
Expand All @@ -75,13 +75,12 @@ type Handler struct {
}

// NewHandler returns a new instance of handler with routes.
func NewHandler(requireAuthentication, loggingEnabled bool, version string) *Handler {
func NewHandler(requireAuthentication, loggingEnabled bool) *Handler {
h := &Handler{
mux: pat.New(),
requireAuthentication: requireAuthentication,
Logger: log.New(os.Stderr, "[http] ", log.LstdFlags),
loggingEnabled: loggingEnabled,
version: version,
}

h.SetRoutes([]route{
Expand Down Expand Up @@ -134,7 +133,7 @@ func (h *Handler) SetRoutes(routes []route) {
if r.gzipped {
handler = gzipFilter(handler)
}
handler = versionHeader(handler, h.version)
handler = versionHeader(handler, h)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I typically prefer to only send what is needed to the receiving method, which in this case is just the version string. It makes the receiving function easy to understand what is actually needed.

handler = cors(handler)
handler = requestID(handler)
if h.loggingEnabled && r.log {
Expand Down Expand Up @@ -662,9 +661,9 @@ func gzipFilter(inner http.Handler) http.Handler {

// versionHeader taks a HTTP handler and returns a HTTP handler
// and adds the X-INFLUXBD-VERSION header to outgoing responses.
func versionHeader(inner http.Handler, version string) http.Handler {
func versionHeader(inner http.Handler, h *Handler) http.Handler {
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.Header().Add("X-InfluxDB-Version", version)
w.Header().Add("X-InfluxDB-Version", h.Version)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related to the above comment, same thing. Restricting scope for arguments is important.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, I see what you did now that I looked at the file as a whole. I guess I'm fine making the signatures consistent. Ignore my previous comments :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually the reason for it is that at the time versionHeader is called h.Version has not been set. so I passed a pointer to h and by the time the HandlerFunc is called, h.Version has been set. there were alternatives to this but I found this one to be my preference.

inner.ServeHTTP(w, r)
})
}
Expand Down
3 changes: 2 additions & 1 deletion services/httpd/handler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -381,10 +381,11 @@ type Handler struct {
// NewHandler returns a new instance of Handler.
func NewHandler(requireAuthentication bool) *Handler {
h := &Handler{
Handler: httpd.NewHandler(requireAuthentication, true, "0.0.0"),
Handler: httpd.NewHandler(requireAuthentication, true),
}
h.Handler.MetaStore = &h.MetaStore
h.Handler.QueryExecutor = &h.QueryExecutor
h.Handler.Version = "0.0.0"
return h
}

Expand Down
1 change: 0 additions & 1 deletion services/httpd/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ func NewService(c Config) *Service {
Handler: NewHandler(
c.AuthEnabled,
c.LogEnabled,
"FIXME",
),
Logger: log.New(os.Stderr, "[httpd] ", log.LstdFlags),
}
Expand Down