Allow server URLs with custom paths or no trailing slash.

[juliusv]

This enables people to use PromDash behind a proxy with a custom path,
like http://myproxy/prometheus.

[juliusv]

@stuartnelson3

[juliusv]

When looking at adding tests, I was going back and forth between either adding three tests (one for each controller/service), which would also require me to figure out how to test successful graph/pie/metric-name loads, or extracting the URL concatentation code into a separate service and testing that once. Both seemed like a lot of overhead for such a simple orthogonal change though. Let me know if you think otherwise...

[grobie]

It must not be hard to add tests or feel like it's an overhead. That means
we need more work on the test suite. Almost all changes are itself simple,
the combination of all these simple things make a program eventually
complex and is the reason we need a test suite.

I vote to extract all the HTTP request handling here and have a common
service for that. Regardless of tests, doing the same change N times (3
right now, but will we remember this when the fourth endpoint is added?)
doesn't seem to be right. For #341 it's also necessary to set the
Authentication header correctly for prometheus server requests
https://docs.angularjs.org/api/ng/service/$http.

On Sat, Feb 14, 2015 at 12:41 PM, juliusv [email protected] wrote:

When looking at adding tests, I was going back and forth between either
adding three tests (one for each controller/service), which would also
require me to figure out how to test successful graph/pie/metric-name
loads, or extracting the URL concatentation code into a separate service
and testing that once. Both seemed like a lot of overhead for such a simple
orthogonal change though. Let me know if you think otherwise...

Reply to this email directly or view it on GitHub
#343 (comment).

[juliusv]

Agreed, together with the authentication header change for #341 it would make much more sense to split this out into a separate HTTP service, and then test that. I'm currently struggling to get the combination of Angular/CORS/Basic Auth working... it's a pain. The headers are just not set, etc.

[juliusv]

(oh yeah, and with nginx as the basic auth proxy in the middle)

[stuartnelson3]

I'll add on to what @grobie said: it being hard to write a test is usually indicative of writing our code incorrectly. obviously we don't have enough hours in the day to fix everything, but I do intend on spending more time on the test suite.

also, i'll review this tomorrow :P

[stuartnelson3]

@juliusv added tests

[juliusv]

Oh, thanks so much! :) Adding some tiny comments now...

[juliusv]

Actually, just 👍

Once we do end up adding basic auth, we probably want to not only extract the URL building, but the whole HTTP call. But we can leave that for then.

[grobie]

I'm confused. First you have a s variable which can contain two different domain names, but then you override s to this server name only? Or what am I missing here?

[juliusv]

Oh, good catch. Yeah, looks like this line (and the same one in the test below) should be deleted?

[stuartnelson3]

ahhh this is true. when I expanded the test to have the two different server names, i forgot to delete the original s var.