-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Inject version info into index.html #2547
Inject version info into index.html #2547
Conversation
Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: Yuri Shkuro <github@ysh.us>
b9da426
to
05df5b8
Compare
Codecov Report
@@ Coverage Diff @@
## master #2547 +/- ##
==========================================
+ Coverage 95.32% 95.37% +0.04%
==========================================
Files 208 208
Lines 9248 9251 +3
==========================================
+ Hits 8816 8823 +7
+ Misses 355 352 -3
+ Partials 77 76 -1
Continue to review full report at Codecov.
|
GitVersion string `json:"GitVersion"` | ||
BuildDate string `json:"BuildDate"` | ||
GitVersion string `json:"gitVersion"` | ||
BuildDate string `json:"buildDate"` | ||
} |
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.
This could be considered a minor breaking change, I don't know why there was a mismatch in the name casing.
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.
This JSON output is used in version
command and /version
endpoint:
$ curl http://localhost:14269/version
{"gitCommit":"","gitVersion":"","buildDate":""}
Hard to imaging there would be dependencies on the exact string output of these.
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.
I really wouldn't worry about it...
if options.BasePath == "" { | ||
options.BasePath = "/" | ||
} | ||
if options.BasePath != "/" { | ||
if !strings.HasPrefix(options.BasePath, "/") || strings.HasSuffix(options.BasePath, "/") { | ||
return nil, fmt.Errorf(errBadBasePath, options.BasePath) | ||
return nil, fmt.Errorf("invalid base path '%s'. Must start but not end with a slash '/', e.g. '/jaeger/ui'", options.BasePath) |
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.
How about replacing '%s'
with %q
?
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.
I think it only makes the logs uglier because JSON logger will have to escape quotes, etc.
GitVersion string `json:"GitVersion"` | ||
BuildDate string `json:"BuildDate"` | ||
GitVersion string `json:"gitVersion"` | ||
BuildDate string `json:"buildDate"` | ||
} |
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.
I really wouldn't worry about it...
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.
Looks good to me.
Part of jaegertracing/jaeger-ui#365