-
-
Notifications
You must be signed in to change notification settings - Fork 197
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
Utilize builtin go "embed" package to embed all static files needed by internal viewer UI #785
Conversation
…y internal viewer UI
I'm a big fan of this PR, but we're in a peculiar situation where we need to keep supporting the old way of embedding assets. In an ideal situation, we could use go build flags to sniff the version of Go that's being used for building and use the cc @gdey |
…ernal viewer when compiled with go1.16+
# Conflicts: # server/viewer.go # ui/embed.go
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 looks good to me. No real comments. @gdey any feedback? You have more experience with embed
.
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.
Need to look at the build failure. We may need to move the build of the assets around; seems to be failing to find the dist
dir according to the CI. (This is when it's running the tests; so we may need to just include the dist dire and a file)
ui/embed.go
Outdated
@@ -0,0 +1,23 @@ | |||
// +build !noViewer go1.16 |
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.
Then shouldn't this be !noViewer,go1.16
?
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 have now completely omitted the +built constraint in embed.go
It doesn't seem to matter because "embed.go" is only referred in "viewer_embed.go" which is only considered when built with go 1.16+
It compiled fine with no tag set and with -tag NoViewer on Go1.15.14.
…when using embed package in Go 1.16
Pull Request Test Coverage Report for Build 6dfae95f3-PR-785
💛 - Coveralls |
@mapl can you squash these commits when you have a chance? this is just about ready to be merged. |
I made some git mistakes and decided to open another PR for this. |
dist folder not included in source