Add CI check for vtadmin-web proto/typegen drift

doeg commented 2 years ago

tl;dr

Right now, it's up to humans to remember to run make vtadmin_web_proto_types after modifying vtadmin.proto or any of its dependencies. For branches that modify at least one proto/*.proto file, I propose adding a CI check for drift between the generated types on the PR branch vs. the main branch.

Longer version

The vtadmin_web_proto_types makefile target generates TypeScript declarations + .js wrappers from vtadmin.proto, into vtadmin-web/src/proto/.

Anytime vtadmin.proto or any of its .proto dependencies are modified, it's necessary to run make proto (as usual) and then make vtadmin_web_proto_types.

Forgetting this step can result in vtadmin-web runtime errors, since vtadmin-web will be using different generated types, compared to vtadmin-api and the rest of Vitess.

This, combined with a CI check to confirm the front-end build is green (https://github.com/vitessio/vitess/pull/8993) should avoid situations like https://github.com/vitessio/vitess/pull/8623 and https://github.com/vitessio/vitess/pull/8566. In both cases, I wasn't around to review and it (understandably!!!) wasn't clear to the PR authors whether or not the changes broke/otherwise affected vtadmin-web.

@ajm188 mentioned that a proto diff/drift check could be more widely useful to other makefile targets. I wanted to keep this ticket scoped fairly narrowly to vtadmin-web since it's less work for me 😈 but if it's applicable elsewhere, then that's great + I'd love to know about it!

A solution + an alternative

Two potential solutions, both of which will address the problem of drift between vtadmin.proto and the front-end codegen. ...Well, more like: one good solution and then a less-good alternative that I will mention for completeness. :)

✨ Proposed solution: Add a "proto drift check" in CI that runs make proto and make vtadmin_web_proto_types, and compares the resulting TypeScript/JavaScript codegen with the file on the branch. The check will fail if there are any diffs between the generated-in-CI version and the branch version. If the check fails, the dev will (for now) need to run make vtadmin_web_proto_types locally and then push up the newly generated files.

Pros:

As @ajm188 mentioned in our conversation, we could (now or later) broaden this check (or add a new one) to run a similar diff/drift check against other generated (golang) code.
Devs won't need node/npm/other front-end dependencies to run make proto unless the change would cause drift and/or legitimately break the front-end build.

Cons:

Setting up a drift check is a nonzero amount of work. (But I'm happy to do it!)
It might be tricky to make the check only run if vtadmin.proto or any of its dependencies are edited, since that would require knowing which subset of the protos are used by vtadmin. Not impossible, but walking a dependency tree seems like overkill when we could (more easily, at the expense of a wider net) run the check on changes to any proto/*.proto file.
Fixing the build will necessitate running make vtadmin_web_proto_types somewhere. (If people are really resistant to installing the front-end dependencies, we can figure that out, too.) Not a con, per se, but worth mentioning.

☹️ Rejected unpleasant alternative: the vtadmin_web_proto_types target to the make proto target (or add a new target), so that it will always be run without having to remember to do so.

Pros:

Humans won't have to remember to run make vtadmin_web_proto_types

Cons:

make proto will have a "dependency" on the vtadmin_web_proto_types target, and therefore will have a dependency on the correct node version being installed. I can understand why this would be a big con for backend devs. :)
vtadmin_web_proto_types would be run on any .proto file change, even if it's not a dependency of vtadmin.proto.
Probably more cons beyond these since overall this seems cumbersome and annoying!

ajm188 commented 2 years ago

I think I'm not convinced we should reject "make proto depends on vtadmin_web_proto_types target"

Devs should really only be running that if they change a .proto file, and if they change (admittedly some, but not all) a .proto file then we want to make sure the vtadmin types are up-to-date as well. So if we reject that alternative, then the drift check will fail in CI, and they will need to have a node version installed in order to fix it anyway, so I think the main con is actually an unavoidable cost just in general.

I think we should have a drift check in addition, btw! Most of the codegen does that (make target + drift check), and protos are the only notable exception, and if I had to guess it would be because those existed long before github actions and we started wanting to check drift.

doeg commented 2 years ago

Hmmmm that's very true!

@deepthi I'm curious what you think, too. Adding node as developer dependency would be a fairly big change, but Andrew's right that it's effectively unavoidable in order to keep vtadmin in sync (both in protos + in fixing the build if needed).

One thing worth mentioning for completeness is that there are other proto-to-TypeScript generators out there such as ts-protoc-gen and ts-proto. I've only found one library, protoc-gen-typescript, that seems to only require go (no node), but it doesn't look like it would work out-of-the-box and it's not very widely used. So... I'd strongly prefer we stick protobufjs.

vitessio / vitess