Skip to content
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

Use Check Runs API to report on status checks (success and failures) #1129

Merged
merged 7 commits into from
Jun 24, 2024

Conversation

BillWagner
Copy link
Member

@BillWagner BillWagner commented Jun 6, 2024

Replaces #1128 (because checks: write permissions is honored when the permission is added on a fork - details below)

This updates the TOC / Anchor update tool to create a Status Check payload and upload that as part of the workflow. A new library classes captures all warnings, errors, and information messages and adds them to an "annotations" collection. When the workflow completes, the annotations and status are sent to GitHub as a status check. That enables the errors and warnings from this tool to be displayed inline in the files tab on the PR.

My tests using #1128 failed because this PR adds the checks:write permission to this workflow. As a security precaution, when a PR from a fork adds write permissions to any workflow, those permissions are disabled in the PR. That prevents a malicious user from elevating their permissions.

@BillWagner
Copy link
Member Author

Woo hoo:

image

Prototype of generating checks

This has a prototype to create the proper JSON format for the status check.

Remaining task: Consume the output and POST the results.
POST the check results back to GitHub
@BillWagner BillWagner marked this pull request as ready for review June 6, 2024 18:44
This way, the only annotations that are displayed actually matter for the tool being run
Copy link
Member Author

@BillWagner BillWagner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added notes for reviewers.

tools/StandardAnchorTags/DiagnosticIDs.cs Show resolved Hide resolved
tools/StandardAnchorTags/Program.cs Show resolved Hide resolved
tools/Utilities/StatusCheckLogger.cs Show resolved Hide resolved
tools/run-section-renumber.sh Show resolved Hide resolved
@BillWagner
Copy link
Member Author

ping @jskeet for review.

Once this one is merged, I'll make similar changes to the other tools.

@jskeet
Copy link
Contributor

jskeet commented Jun 21, 2024

Will have a look on Monday - busy as a dad taxi today I'm afraid.

Copy link
Contributor

@jskeet jskeet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing to check on whether warnings should count as errors, but other than that looks okay. I can't say I've reviewed hugely thoroughly.

tools/Utilities/StatusCheckLogger.cs Outdated Show resolved Hide resolved
Warnings shouldn't be treated as errors.
@BillWagner
Copy link
Member Author

I did update to succeed with warnings.

We'll see if we want to replace that.

@BillWagner BillWagner merged commit 6bcec3b into draft-v8 Jun 24, 2024
7 checks passed
@BillWagner BillWagner deleted the integrate-output branch June 24, 2024 14:53
@BillWagner
Copy link
Member Author

Contributes to #1125

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants