Skip to content
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

Embed vtctld assets #5597

Merged
merged 10 commits into from
Jan 2, 2020
Merged

Conversation

rohit-nayak-ps
Copy link
Contributor

@rohit-nayak-ps rohit-nayak-ps commented Dec 19, 2019

Part of #5502

Embedded web/vtctld2 web application assets into vtctld using go.rice.
Hotfixed javascript to fix an issue where a polyfill was causing some versions of chrome not to load the application.
Removed web/vtctld and related code (checked with Sugu, it has been deprecated for some time).
Removed web_dir/web_dir2 config parameters from examples/local/vtctld-up.sh

Signed-off-by: Rohit Nayak <rohit@planetscale.com>
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
@morgo
Copy link
Contributor

morgo commented Dec 19, 2019

I checked out your branch. Just a couple of quick suggestions:

  • Add the rice file to gitignore
  • Add the embed_static to the build target.
  • Change the rice command to be a go run command, so rice doesn't need to be installed first.

Here's a patch:

diff --git a/.gitignore b/.gitignore
index be8fe7275..a4095ee9d 100644
--- a/.gitignore
+++ b/.gitignore
@@ -85,3 +85,6 @@ releases
 /vthook/
 /bin/
 /vtdataroot/
+
+# Rice generated files
+/go/vt/vtctld/rice-box.go
diff --git a/Makefile b/Makefile
index 54fb5df29..2ec325966 100644
--- a/Makefile
+++ b/Makefile
@@ -39,7 +39,7 @@ endif
 
 embed_static: 
        cd go/vt/vtctld
-       rice embed-go
+       go run github.com/GeertJohan/go.rice/rice embed-go
        go build .
 
 build_web:
@@ -47,7 +47,7 @@ build_web:
        cd web/vtctld2 && ng build -prod
        cp -f web/vtctld2/src/{favicon.ico,plotly-latest.min.js,primeui-ng-all.min.css} web/vtctld2/dist/
 
-build:
+build: embed_static
 ifndef NOBANNER
        echo $$(date): Building source tree
 endif

Otherwise LGTM

@morgo morgo self-requested a review December 19, 2019 21:46
…Also removed web_dir(2) from docker/K8s yaml configs

Signed-off-by: Rohit Nayak <rohit@planetscale.com>
@rohit-nayak-ps
Copy link
Contributor Author

Adding embed_static to the build step causes the rice file to be rebuilt on every make and presumably vtctld and dependent packages get rebuilt too. Is it ok to add this overhead?

@morgo
Copy link
Contributor

morgo commented Dec 20, 2019

Adding embed_static to the build step causes the rice file to be rebuilt on every make and presumably vtctld and dependent packages get rebuilt too. Is it ok to add this overhead?

I am okay with it - it didn't seem like it took that long. The alternative is that we commit the rice-boxes (remove from gitignore). The problem I can see with this approach is that there will very likely be merge conflicts if multiple users are editing the source files. @sougou or @deepthi did you have opinions?

@morgo
Copy link
Contributor

morgo commented Dec 20, 2019

I chatted with sougou and deepthi - lets go with how you have it (regen as a manual step). Make sure you commit the generated file /go/vt/vtctld/rice-box.go to the repo so that users won't have to generate it to build.

…o git

Signed-off-by: Rohit Nayak <rohit@planetscale.com>
@morgo
Copy link
Contributor

morgo commented Dec 20, 2019

@rohit-nayak-ps still a conflict on go.mod/go.sum.

@guidoiaquinti
Copy link
Member

Do we need to also remove the references from 1 and 2?

@morgo
Copy link
Contributor

morgo commented Feb 1, 2020

Do we need to also remove the references from 1 and 2?

I believe we will during Vitess 6.0 development. We discovered compatibility issues by removing these options too soon, so the behavior was changed to parse-but-ignore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants