traceapp: take a base URL parameter in New #162
Conversation
This change requires that users of traceapp pass a base URL parameter. This is needed in order to create absolute URLs to e.g. traces for permalinks. Prior to this change, we would get this base URL via `github.com/gorilla/mux.Route.URLTo` but this notably enforces an HTTP-only scheme ([source code here](https://github.com/gorilla/mux/blob/master/route.go#L473)) which prevents Appdash permalinks from working on HTTPS hosted systems. This is fixed with this change, by requiring an explicit base URL to be given as a parameter.
| 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()) |
There was a problem hiding this comment.
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.
Good catch, I've changed port to host.
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?
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.
|
If I have a few inline comments, but otherwise LGTM |
… on --http addr). This causes `appdash serve` to always choose a better default for `--url` when not specified. Prior to this it would choose a hard-coded default which matched the `--http` default, but that default would not work when changing `--http`.
At an initial glance this would seem like a simplification, but it's actually a bad idea in disguise. It's actually very important that users specify the right base URL otherwise some things will break (e.g. trace permalinks, certain JS/CSS/etc file includes, etc) and others won't. It's not pretty, and worse might not always be obvious in the future, so best to force the user to think about what exactly that parameter really means. |
This change requires that users of traceapp pass a base URL parameter. This is
needed in order to create absolute URLs to e.g. traces for permalinks. Prior to
this change, we would get this base URL via
github.com/gorilla/mux.Route.URLTobut this notably enforces an HTTP-only scheme (source code here)
which prevents Appdash permalinks from working on HTTPS hosted systems. This is
fixed with this change, by requiring an explicit base URL to be given as a
parameter.
Verified as working manually via the examples.