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

Add front-end build tooling + fix build error #6473

Merged
merged 2 commits into from
Jul 27, 2020
Merged

Add front-end build tooling + fix build error #6473

merged 2 commits into from
Jul 27, 2020

Conversation

doeg
Copy link
Contributor

@doeg doeg commented Jul 25, 2020

The Problem

In April, an errant paren was introduced which broke the build for the vtctld2 UI. Since then, making changes to the vtctld2 UI has required editing the minified .bundle.js and CSS files directly. Fixing this bug is complicated by the very old (and undocumented) versions of node and npm required by the build.

What this PR does

  • Adds web_bootstrap.sh to check for the correct versions of node and npm before running npm install.

  • Fixes the extra paren that's breaking the build once dependencies are installed.

  • Improves the local dev workflow by adding a proxy pass-through for API requests from an Angular server (which offers hot reloading) to an instance of the vtctld API running in Docker. The proxy layer lets us side-step any CORS issues (ugh) we might run into otherwise. This is convenient as it gives our UI data to work with, without needing to add a bunch of API response mocks to the front-end.

  • Streamlines (and documents) the web Makefile targets a little bit.

🌶️ Importantly, this PR does NOT regenerate the production vtctld UI assets. In other words, merging this PR will not affect the current UI in any way.

This is for a few reasons:

  • To reduce the surface area + risk of merging this PR

  • To avoid overwriting any changes that were made directly to the current minified .bundle.js files

  • To make rollbacks easier: if, separately, we regenerate the production build and run into issues, it will be much easier to roll that back without losing the tooling improvements in this PR.

  • I'm not familiar with the vtctld2 UI (yet); it would take me some time to verify that everything still works as intended. :)

Reproducing the build error

First, follow the installation instructions to acquire the right version of node & npm. (This can be automated, but I frankly don't think it's worth the time if the intention is to rewrite most of this.)

Then, check out this branch (so we can use the updated build tooling) and reintroduce the build error:

git checkout sarabee-vtctld-ui-build
git checkout master web/vtctld2/src/app/dashboard/shard.component.html

Then, do a production build of the front-end assets and run it all within Docker (or outside of Docker, if you prefer, as long as the API is hosted on http://localhost:15000). This is also documented in the README.

source dev.env
make web_build
make docker_local && ./docker/local/run.sh

Finally, open up the hosted vtctld2 UI on http://localhost:15000/app. You'll see a "Loading..." message and a bunch of template errors in the Chrome console:

vitessio-template-error

You can kill the Docker server at this point with ctrl+C.

Testing out this fix

Remove the bug and generated files we reintroduced above:

git checkout -f && git clean -fd

Do another production build, as above (and in the README):

make web_build
make docker_local && ./docker/local/run.sh

You should then see the UI on http://localhost:15000/app 🎉

vitessio-fix

What else?

  • I would very appreciate it if someone would take the time to verify that the dev server works, too. Instructions for that are here. Here's what it looks like in action:

vitess-docker-proxying

  • Previously, I said I'd do the terminology updates as part of this work. I'd actually like to do those separately to keep this PR as minimal as possible.

Open to any and all feedback. 🙏 Thanks for looking, and extra thanks to @acharis for talking through some of the go parts with me. :)

@doeg doeg requested a review from sougou as a code owner July 25, 2020 18:58
Signed-off-by: Sara Bee <855595+doeg@users.noreply.github.com>
Signed-off-by: Sara Bee <855595+doeg@users.noreply.github.com>
@sougou
Copy link
Contributor

sougou commented Jul 27, 2020

Nice work! I was able to build it locally (I think). We can merge after @rohit-nayak-ps takes a look.

Copy link
Contributor

@rohit-nayak-ps rohit-nayak-ps left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great. I was able to setup the tooling and make some trivial local changes to validate.

The two new files have 2019 as the copyright date, but maybe we can fix that later to meet the release cut-off ...

@doeg
Copy link
Contributor Author

doeg commented Jul 27, 2020

@rohit-nayak-ps @sougou previously, @deepthi and I had talked about getting the terminology changes in for the next release. I'm happy to update the copyright date along with the terminology changes and open a PR with both sets of changes today -- I should be able to get that done before 3:00pm PST. :)

@deepthi deepthi merged commit befc0fb into vitessio:master Jul 27, 2020
@deepthi deepthi added this to the v7.0 milestone Jul 27, 2020
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.

4 participants