-
Notifications
You must be signed in to change notification settings - Fork 10
feat: add validity check to Electron Archaeologist runs #42
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
base: main
Are you sure you want to change the base?
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? |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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