feat: add validity check to Electron Archaeologist runs #42
Conversation
|
@alicelovescake this repo's circleci config is used for the TypeScript comparison - it doesn't run tests at the moment. Potentially it might be easier to extract that aspect to a GitHub Action like the lint job? |
alicelovescake
force-pushed
the
alice-add-valid-check
branch
from
75fd6ba to
3dfe6df
Compare
Thanks for the pointer! Yeah I wasn't confident whether to put the test in the circle ci or github. Updated PR with new github workflow. |
| conclusion: CheckRunStatus.FAILURE, | ||
| title: 'Label Mismatch with Changes Detected', | ||
| summary: "Changes detected despite the presence of 'semver/none' label. ", |
There was a problem hiding this comment.
There needs to be a way of overriding this
| conclusion: CheckRunStatus.FAILURE, | ||
| title: 'Label Mismatch with No Changes', | ||
| summary: "No changes detected despite the presence of 'semver/minor' or 'semver/major' labels.", |
There was a problem hiding this comment.
There needs to be a way of overriding this, something like like our fast-track or skip-backport-check labels to bypass trop / cation checks.
This isn't 100% provably correct, for instance:
- You can make a
semver/majorchange with 0 API surface changing, for instance enabling a feature by default (Cooke Encryption comes to mind as a semver/major with 0 API) - You can make a
semver/nonechange that modifies the API surface. For instance docs fixes commonly "modify" theelectron.d.tsfile but don't actually change the API surface. So unless this check gets smarter around diffing actual API surface rather than just theelectron.d.tsfile we need a way to say "in this case we're all good"
There was a problem hiding this comment.
Thanks for pointing out the edge cases that I haven't considered!
I can add a new label that's skip-type-diff-check but do you think it adds unnecessary complexity to the process? After all, the primary purpose of these checks is to provide informational cues to users.
What if for expected valid changes, we set the status to CheckRunStatus.SUCCESS and for suspected invalid changes, we set the status to CheckRunStatus.NEUTRAL with a summary that gives a warning that unexpected changes exist but feel free to skip if it's intentional.
This way, it serves the purpose of providing information without adding process overhead. If we feel like the override tag is better, happy to add it!!
This PR fixes #41 :
getCheckStatusItemsto handle the check status based on the presence of changes and semantic versioning labels in pull requests.getCheckStatusItemsfunction under various conditions.Note: Not 100% CI configs are correct, mostly copied over from Trop