This repository has been archived by the owner on Oct 29, 2021. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 137
traceapp: take a base URL parameter in New #162
Merged
Merged
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
11087ea
traceapp: take a base URL parameter in New
emidoots 331a864
cmd/appdash: upgrade to new traceapp.New API
emidoots a6db917
example/cmd: upgrade to new traceapp.New API
emidoots ddc65a3
CHANGELOG: mention traceapp.New change
emidoots 17c8403
traceapp: s/port/host/g typo
emidoots 48283ad
traceapp: properly copy the base URL parameter in New
emidoots cbf71cd
cmd/appdash: choose better `appdash serve --url` default value (based…
emidoots File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,6 +17,7 @@ import ( | |
"encoding/base64" | ||
"encoding/json" | ||
"errors" | ||
"fmt" | ||
htmpl "html/template" | ||
"io/ioutil" | ||
"log" | ||
|
@@ -45,17 +46,35 @@ type App struct { | |
tmplLock sync.Mutex | ||
tmpls map[string]*htmpl.Template | ||
|
||
Log *log.Logger | ||
Log *log.Logger | ||
baseURL *url.URL | ||
} | ||
|
||
// New creates a new application handler. If r is nil, a new router is | ||
// created. | ||
func New(r *Router) *App { | ||
// | ||
// The given base URL is the absolute base URL under which traceapp is being | ||
// served, e.g., "https://appdash.mysite.com" or "https://mysite.com/appdash". | ||
// The base URL must contain a scheme and host, or else an error will be | ||
// returned. | ||
func New(r *Router, base *url.URL) (*App, error) { | ||
if r == nil { | ||
r = NewRouter(nil) | ||
} | ||
|
||
app := &App{Router: r, Log: log.New(os.Stderr, "appdash: ", log.LstdFlags)} | ||
// Validate the base URL and use the root path if none was specified. | ||
if base.Scheme == "" || base.Host == "" { | ||
return nil, fmt.Errorf("appdash: base URL must contain both scheme and port, found %q", base.String()) | ||
} | ||
if base.Path == "" { | ||
base.Path = "/" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd document that you potentially modify There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changed it to make a copy. |
||
} | ||
|
||
app := &App{ | ||
Router: r, | ||
Log: log.New(os.Stderr, "appdash: ", log.LstdFlags), | ||
baseURL: base, | ||
} | ||
|
||
r.r.Get(RootRoute).Handler(handlerFunc(app.serveRoot)) | ||
r.r.Get(TraceRoute).Handler(handlerFunc(app.serveTrace)) | ||
|
@@ -71,7 +90,7 @@ func New(r *Router) *App { | |
// Static file serving. | ||
r.r.Get(StaticRoute).Handler(http.StripPrefix("/static/", http.FileServer(static.Data))) | ||
|
||
return app | ||
return app, nil | ||
} | ||
|
||
// ServeHTTP implements http.Handler. | ||
|
This file contains 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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you are checking host is non-nil, but then complain about the port missing.
Also, can we not just trust that the user passed in a reasonable URL? We don't actually use the scheme or host specifically right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, I've changed
port
tohost
.Not entirely. The whole point of this change is that we are explicitly using a scheme and host in order to link to trace permalinks. This is because traceapp really can't know where it is hosted unless someone tells it.
We could omit the check for scheme and host.. but it would make it a lot easier for users to shoot themselves in the foot.