Comment PRs with updated schedule information #1576

djmitche · 2023-12-11T18:03:21Z

This adds a GH action to add a comment to every PR giving the updated course schedule with the PR merged.

To accomplish this, I broke mdbook-course into a library and two binaries, allowing the mdbook content to be loaded dynamically outside of an mdbook build invocation.

I think this is a net benefit, but possible improvements include:

diffing the "before" and "after" schedules and only making the comment when those are not the same (or replacing the comment with "no schedule changes")
including per-segment timing behind <details> (with a few minutes effort I couldn't get this to play nicely with the markdown lists)

github-actions · 2023-12-11T18:03:36Z

Course Schedule

With this pull request applied, the course schedule is as follows:

Fundamentals

21 hours and 45 minutes: (2 hours and 15 minutes short)

Day 1 Morning - 3 hours
Day 1 Afternoon - 2 hours and 55 minutes
Day 2 Morning - 3 hours and 15 minutes (⏰ 15 minutes too long)
Day 2 Afternoon - 3 hours
Day 3 Morning - 2 hours and 15 minutes: (45 minutes short)
Day 3 Afternoon - 2 hours and 20 minutes: (40 minutes short)
Day 4 Morning - 3 hours and 10 minutes (⏰ 10 minutes too long)
Day 4 Afternoon - 2 hours: (1 hour and 5 minutes short)

djmitche · 2023-12-11T19:28:27Z

.github/workflows/course-schedule.yml

@@ -0,0 +1,25 @@
+name: "Course Schedule Updates"
+on:
+  - pull_request_target


Note that the schedule in this PR won't update, since this directive indicates that the action should run based on configuration in the target branch -- and it hasn't been merged yet.

We should probably think about the implications of this!

We use

on: pull_request:

in the build.yml. The difference is that pull_request run the action in the context of the (future) merge commit, whereas pull_request_target runs at the base of the branch.

Are you running it at the base to enable the read/write behavior of the GITHUB_TOKEN?

Yes. But, I didn't realize that the checkout will check out the base as well. That means we don't get the info on the timings in src/ without more effort.

We could do a git checkout $rev src/ of the PR revision, but that relies on mdbook parsing as a security boundary, which is probably not adequate.

Maybe there's a way to split this into two actions -- one that runs on pull_request and does the analysis, and one that runs on pull_request_target and just posts the resulting analysis to a comment. I'll look into that.

I don't think it's necessary to split it. Pull requests from new contributors have to be approved before they run — precisely to protect against injection of malicious build steps.

So it should be fine to add the necessary permissions: key to a regular build: we check that the job doesn't change in strange ways before we approve PR builds for new contributors.

That's the theory, as far as I understand it 😄 The real problem is if this understanding is wrong and/or if it's not shared with others who have write access to the repository.

djmitche · 2023-12-14T22:31:11Z

Hm, it appears that jobs from pull_request and pull_request_target cannot have inter-dependencies because they are in different workflows. So I can't upload an artifact in a pull_request workflow and download it in pull_request_target.

I think reusing workflows is more of a subroutine call, and can't cross security boundaries, which is why we need two workflows here.

Artifacts are handled as ZIP files (e.g., https://github.com/google/comprehensive-rust/actions/runs/7215251310) so we can't just link to an artifact and hope it springs into existence before someone clicks the link.

Are there any GitHub actions gurus who might be able to give me a hint here?

djmitche · 2023-12-19T16:17:23Z

Per discussion with @mgeisler I've updated this to just do the simple thing, and rely on reviewers verifying PRs and clicking the run-actions button to check for malicious behavior.

djmitche · 2023-12-27T21:27:31Z

@mgeisler do could you have another look here?

mgeisler · 2024-01-12T14:53:51Z

@mgeisler do could you have another look here?

Yes, let me get this reviewed!

mdbook-course/Cargo.toml

mgeisler · 2024-01-12T15:13:54Z

.github/workflows/course-schedule.yml

+    permissions:
+      pull-requests: write


From Permissions for the GITHUB_TOKEN, I think this will be ineffective for PRs that come from forks.

I guess we'll figure it out soon after merging it 😄

Maybe I misunderstood earlier suggestions - does this not run when we click "Run Actions" for an untrusted fork?

It does run, but it fails because the token is downgraded. That's how I interpret the error I just saw in #1693:

This makes me think that we should remove this again, sorry!

mgeisler

Thanks, I'm looking forward to seeing this in action.

.github/workflows/course-schedule.yml

Co-authored-by: Martin Geisler <[email protected]>

djmitche · 2024-01-12T15:44:01Z

Hm, do I need to hand-apply the diff from that "Test / format" failure? dprint fmt doesn't fix it.

mgeisler · 2024-01-14T17:11:50Z

Hm, do I need to hand-apply the diff from that "Test / format" failure? dprint fmt doesn't fix it.

It should, dprint fmt should update the files in your working copy.

mgeisler · 2024-01-15T08:28:12Z

Hm, do I need to hand-apply the diff from that "Test / format" failure? dprint fmt doesn't fix it.

It should, dprint fmt should update the files in your working copy.

Will be fixed with #1694.

The GitHub action fails because it cannot run with the necessary permissions: it requires write access to be able to post a comment, but when triggered from a fork, the token is limited to read access. The error is from the thollander/actions-comment-pull-request action: No comment has been found with asked pattern. Creating a new comment. Error: Resource not accessible by integration The downgrading of the token is explained on: https://docs.github.com/en/actions/security-guides/automatic-token-authentication where “Maximum access for pull requests from public forked repositories” says that the pull-requests permission is capped at read in the typical case for us. I kept the underlying functionality since we might want to use it for something else (perhaps track the duration in the README). Fixes the errors seen on the recent PRs: #1693, #1687, and likely others.

The GitHub action added in #1576 fails because it cannot run with the necessary permissions: it requires write access to be able to post a comment, but when triggered from a fork, the token is limited to read access. The error is from the thollander/actions-comment-pull-request action: No comment has been found with asked pattern. Creating a new comment. Error: Resource not accessible by integration The downgrading of the token is explained on: https://docs.github.com/en/actions/security-guides/automatic-token-authentication where “Maximum access for pull requests from public forked repositories” says that the pull-requests permission is capped at read in the typical case for us. I kept the underlying functionality since we might want to use it for something else (perhaps track the duration in the README). Fixes the errors seen on the recent PRs: #1693, #1687, and likely others.

djmitche · 2024-01-16T14:08:01Z

Per discussion with @mgeisler I've updated this to just do the simple thing, and rely on reviewers verifying PRs and clicking the run-actions button to check for malicious behavior.

So it seems this was not a working solution :(

mgeisler · 2024-01-16T14:14:43Z

So it seems this was not a working solution :(

Yeah, sorry about backing it out in #1700. The permission system here is rather confusing. I get the impression that a GitHub app is the way to go — but that's probably complex to set up.

This reverts commit 60c5a33.

djmitche force-pushed the pr-commenting branch 3 times, most recently from f2b2310 to 840bcf5 Compare December 11, 2023 19:02

djmitche changed the title ~~Comment PRs with updated schedule information (second try)~~ Comment PRs with updated schedule information Dec 11, 2023

djmitche requested a review from mgeisler December 11, 2023 19:06

djmitche commented Dec 11, 2023

View reviewed changes

djmitche marked this pull request as ready for review December 11, 2023 20:23

djmitche marked this pull request as draft December 14, 2023 15:27

djmitche force-pushed the pr-commenting branch from f4d8cd5 to 07af655 Compare December 19, 2023 16:16

djmitche marked this pull request as ready for review December 19, 2023 16:18

Show course schedule in PRs

1b1633c

djmitche force-pushed the pr-commenting branch from dc86815 to 1b1633c Compare January 10, 2024 21:31

dprint fmt

d7ef48b

mgeisler reviewed Jan 12, 2024

View reviewed changes

mdbook-course/Cargo.toml Outdated Show resolved Hide resolved

mgeisler reviewed Jan 12, 2024

View reviewed changes

mgeisler approved these changes Jan 12, 2024

View reviewed changes

mgeisler reviewed Jan 12, 2024

View reviewed changes

.github/workflows/course-schedule.yml Outdated Show resolved Hide resolved

Apply suggestions from code review

235e993

Co-authored-by: Martin Geisler <[email protected]>

djmitche enabled auto-merge (squash) January 12, 2024 15:29

dprint fmt

883e85f

manually apply dprint diff

1c4c7c1

djmitche merged commit 4c0833a into main Jan 12, 2024
35 checks passed

djmitche deleted the pr-commenting branch January 12, 2024 15:53

This was referenced Jan 14, 2024

n1 should be 5, not 15 #1693

Merged

Better explanation why futures need to be pinned #1687

Merged

mgeisler mentioned this pull request Jan 15, 2024

Partially revert #1576 #1700

Merged

mgeisler mentioned this pull request Jan 23, 2024

Code coverage uploads sometimes fail google/mdbook-i18n-helpers#148

Closed

djmitche mentioned this pull request Jan 24, 2024

Use secrets to allow PR commenting #1755

Closed

djmitche added a commit to djmitche/comprehensive-rust that referenced this pull request Feb 8, 2024

Revert "Partially revert google#1576 (google#1700)"

b05abda

This reverts commit 60c5a33.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comment PRs with updated schedule information #1576

Comment PRs with updated schedule information #1576

djmitche commented Dec 11, 2023 •

edited

Loading

github-actions bot commented Dec 11, 2023 •

edited

Loading

djmitche Dec 11, 2023

djmitche Dec 11, 2023

mgeisler Dec 14, 2023

djmitche Dec 14, 2023

mgeisler Dec 17, 2023

djmitche commented Dec 14, 2023

djmitche commented Dec 19, 2023

djmitche commented Dec 27, 2023

mgeisler commented Jan 12, 2024

mgeisler Jan 12, 2024

djmitche Jan 12, 2024

mgeisler Jan 14, 2024

mgeisler Jan 14, 2024

mgeisler left a comment

djmitche commented Jan 12, 2024

mgeisler commented Jan 14, 2024

mgeisler commented Jan 15, 2024

djmitche commented Jan 16, 2024

mgeisler commented Jan 16, 2024

Comment PRs with updated schedule information #1576

Comment PRs with updated schedule information #1576

Conversation

djmitche commented Dec 11, 2023 • edited Loading

github-actions bot commented Dec 11, 2023 • edited Loading

Course Schedule

Fundamentals

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djmitche commented Dec 14, 2023

djmitche commented Dec 19, 2023

djmitche commented Dec 27, 2023

mgeisler commented Jan 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgeisler left a comment

Choose a reason for hiding this comment

djmitche commented Jan 12, 2024

mgeisler commented Jan 14, 2024

mgeisler commented Jan 15, 2024

djmitche commented Jan 16, 2024

mgeisler commented Jan 16, 2024

djmitche commented Dec 11, 2023 •

edited

Loading

github-actions bot commented Dec 11, 2023 •

edited

Loading