-
Notifications
You must be signed in to change notification settings - Fork 98
[bots] Move the disable json generation to be in HUD #6676
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
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
890a7e2
to
dc185f8
Compare
dc185f8
to
48c121d
Compare
48c121d
to
9d4da28
Compare
d16b152
to
4e5e7d3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I appreciate the context in the PR description, but since this is a pretty big change having a bit more of the architecture delta described would be really helpful.
As someone rusty on how the disable logic worked, details that would help grok the changes more easily include:
- What logic was already duplicated in HUD? It looks to me like the json generation is still going to be triggered by the same workflow, but instead of executing that code locally it's now going to make an API call to hud and have hud do the work instead.
- You mentioned removing code duplication, but at a quick glance it looks like logic is being moved from one place to another. What part of the logic already existed in HUD and is now being de-duplicated?
- Correspondingly, can you summarize the logic that's actually being moved over to HUD? (the part that wasn't duplicated and thus needed to be re-implemented in typescript?)
Finally:
- Re: this change not being testable in CI anymore...that is sad. Since it's depending on a new endpoint, it's understandable that your CI fails, but it would be really nice to have enough unit testing infra in place that we can get a pretty good signal on any future changes to this code. I would hope this to be the only PR that this workflow is red on.
added to pr body
Added unittests for the typescript |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
We get the list of disabled/unstable test/jobs in a python script, but there is code that parses the same thing in javascript for the bot that comments on all disable/unstable test/job issues, so this removes the python script in favor of a new javascript script that reuses the code used for verifyDisableIssue to generate the disable/unstable test/job jsons.
I broke the unstable/disable job json generation in #6096, so it's actually been empty since then. We've been getting by, and this PR restores it, but I wonder if this just means we can get rid of it
Pros:
Cons:
CI is broken on this PR because it calls an API that this PR adds (which basically means this isn't really testable in CI anymore... but also given that I broke some things months ago and didn't notice, the e2e for this is probably not great in the first place). I tested locally with localhost and with the preview and it looks fine to me