Add CI jobs for ESLint and Typescript checks #1439
Conversation
|
|
micheal-parks
requested review from
stevebriskin,
DTCurrie,
mrloureed and
anjinai
|
Is there a reason this needs to be a separate CI job instead of just modifying one or two lines in the lint-web target where we're already running pretty much the same thing? I'd much prefer to keep this sort of stuff as makefile targets so it's easy for people to run things locally when testing. |
|
@Otterverse I've made them separate jobs to both align with APP and to offer a good UX when submitting PRs. If there's an issue with ESLint or Typechecking then the author gets immediate feedback: The author is also allowed to immediately go to details about the failure that are isolated from all other jobs: This allows us to minimize any red herrings or distractions from real issues. If you have any suggestions for alternate ways to do this, I'm into it since I'm by no means a CI expert 😂 I'm happy to make these commands makefile targets as well. The only reason I didn't is that people working on the frontend usually use npm directly instead of make. For instance:
But if you think that people will find value in running these from make instead, I'd be happy to add them in this PR and have the CI jobs reference them instead. |
|
@micheal-parks So my first concern is that while "people working on the frontend" might do it differently, everyone that works in this codebase has to be able to pass the tests, and there's been lots of confusion lately how the frontend ends up failing tests when "I didn't touch it in my PR!" but because it auto-generates code during build, it's decided to switch an API version or something when it pulls from upstream. So in order to submit anything in RDK, people have to be able to run everything here, even if they aren't a frontend dev. Makefiles also act as a form of documentation, so that new devs can see what commands are typically used in a codebase, and can make it easier to figure out... this is doubly important now that we're open source and our stuff is going to be used by external devs figuring it out for the first time. My second concern is that we're already running buf-web and lint-web targets that do most of this in the main "build lint" test step, and duplicating that just slows down the whole CI process. I'd really like to avoid duplicating work as much as possible. PR test runs already take long enough as-is. Please understand, it's not that your one request here is the whole of the issue... This is only duplicating about 2 minutes worth of steps, but those add up... and I'm just trying to fight the trend of tests getting longer and longer. Side note: I'm really hoping to talk people into doing this: https://viam.atlassian.net/browse/PRODUCT-265 (separate UI code into it's own repo, so both app and rdk pull from there, instead of just rdk>>app.) All that said, it sounds like the main reason you want these isolated is so it's easier to see the output for this step alone. Would it be reasonable to make these seperate steps in the main test job? E.g. instead of running "build lint" that does everything, split it up into steps that do "buf-go, lint-go, buf-web, lint-web, typecheck (if that's not part of lint-web?)" Then nothing we be duplicated, and you'd still have separate steps to pick out the output from. |
yep! sounds good. Will add corresponding makefile commands here.
Yep. I'm very much against long CI or inefficient tasks. I think that quick CI and deploy times ensures a fast debug loop and therefore a healthy development environment. The two new tasks,
Absolutely. Two goals: Typechecking and ES Linting should be two distinct checks or x's that are reported here on the main page. This is because they're two absolutely essential steps to ensure continuation of good code health for our UI. If you have any suggestions for doing this more efficiently I'm happy to follow them. Is your suggestion how we currently do it on app? I'm trying to reproduce that exactly here. |
|
@Otterverse Just updated CI jobs to use make commands! Is something like this what you had in mind? |
It is, vis-a-vis the makefile targets. The other part of what I was suggesting was to break up the current step that is "verify no uncommitted changes from "make build lint" into separate steps for build-go, lint-go, build-web (which includes buf) and then lint-web (which should be eslint as you're doing it here, modifying if needed) and finally adding typecheck. This would be instead of having them as separate jobs, thus avoiding duplicated work. The new jobs you're adding are also duplicating the buf-web stuff of each other. That matters less than the fact that spinning up an instance for a test (with the code checkout and everything, plus cleanup afterwards) is actually the bulk of the work. All that said... I think this is a bigger issue in overall testing layout, not just something to solve here. For now, I'm gonna review and let this move forward, and have a bigger talk in devops with others about how to lay out and optimize all this, and possibly rearrange in the future. |
|
This PR adds CI jobs for ESLint and Typescript. As an immediate follow-up we should make these checks required to merge.
This PR also syncs our ESLint rules with App and cleans up any issues that arose after syncing.
Note: a lot of types, primarily robot component statuses, are still super broken. I had to place a bunch of band-aids on them for now until we can properly refactor. If people introduce breaking changes to statuses, they will still break the client silently.