-
Notifications
You must be signed in to change notification settings - Fork 4.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
Include the Web UI in the consul binary #1274
Conversation
This is really cool! I think Also, please add documentation for this setting to |
516d2a4
to
280044b
Compare
I've re-updated the PR! Though, this is still missing the dependencies on https://github.com/jteeuwen/go-bindata and https://github.com/elazarl/go-bindata-assetfs; also, the travis build file needs to be changed to install the bundler gem and install the needed Web UI dependencies. how should I do it? |
I've added the dependencies installation. The travis build is now failing due to some failing test. I think its not really related to my change. |
Hi @rgl thanks for the PR! Once we get Consul 0.6 rolled out I'll help review this and get it in for the following release. |
Hey @rgl, this definitely looks good. I started the same thing in a branch (see here) before I realized you had already done this. If you want to finish this up I'll just close out my branch. One idea, let's try to avoid the hard requirement on ruby/bundler and compiling the web UI assets in the typical build pipeline for Consul. The UI very rarely changes so I think we should just have a separate make target like |
I would like to finish it up. I think that always generating the file is better for early catching build errors. But I can implement it either way. Lets see what @ryanbreen and @slackpad have to say. |
Yeah I'd go with @ryanuber's suggestion and just check in the go file - adding Ruby/bundler to the default build path is pretty heavy so this'll keep things fast and simple for most people who aren't updating the web UI. |
Seems fair. As the @ryanuber branch already has that done, I'll retract this PR. |
This is to close #1220. I'm not sure if I should have named the internal variable
EnableUi
instead ofUi
. Let me known and I'll change it.