-
-
Notifications
You must be signed in to change notification settings - Fork 5.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
Fix for #828: Embed build tags #1051
Conversation
Add build tags to ldflags and print in version output Signed-off-by: Jonas Östanbäck <jonas.ostanback@gmail.com>
/cc @tboerger |
LGTM |
seems great |
will fix #828 |
main.go
Outdated
if len(Tags) > 0 { | ||
return " built with: " + strings.Join(strings.Split(Tags, " "), ", ") | ||
} | ||
return "" |
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.
func formatBuiltWith(Tags string) string {
if len(Tags) == 0 {
return ""
}
return " built with: " + strings.Join(strings.Split(Tags, " "), ", ")
more readable?
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.
Or strings.Replace(Tags, " ", ", ", -1)
?
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.
Yes, is guess the special case is empty tags. Will update with both suggestions.
Signed-off-by: Jonas Östanbäck <jonas.ostanback@gmail.com>
It didn't work well. Because
It's ok. |
@lunny Don't think I understand where else you mean it's used. Can you explain please? |
I like how it's set now, but I'm not sure if I like how it's getting printed out |
The version also gets printed on the page itself. Maybe we want to print the tags also on the admin page |
main.go
Outdated
} | ||
|
||
return " built with: " + strings.Replace(Tags, " ", ", ", -1) | ||
} |
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.
strings.Join
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.
Tags is sqlite bindata
form. Why use strings.Join ?
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.
True, but maybe it's even easier to create a comma separated list to split by space and join by comma
I'm fine with displaying it as a comma separated list without quotes if I look on the referenced issue. But additionally it should be displayed on the admin ui. |
Signed-off-by: Jonas Östanbäck <jonas.ostanback@gmail.com>
main.go
Outdated
func init() { | ||
setting.AppVer = Version | ||
setting.AppVer = Version + formatBuiltWith(Tags) |
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 should be done with a separate variable, IMHO we should not list the build tags on every page in the footer, only within the admin dashboard
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.
True, I missed that!
Signed-off-by: Jonas Östanbäck <jonas.ostanback@gmail.com>
build failed. |
modules/templates/helper.go
Outdated
@@ -48,6 +48,9 @@ func NewFuncMap() []template.FuncMap { | |||
"AppVer": func() string { | |||
return setting.AppVer | |||
}, | |||
"AppBuiltWith": func() string { | |||
return setting.AppBuiltWith | |||
}, |
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.
coding style.
LGTM |
Add build tags to ldflags and print in version output