-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Improved support for commenting when course timings change. #1797
Conversation
This * Works with the GitHub permissions model (at least in local testing on another temporary repo) * Only comments when the timings actually change
mkdir -p ./course-schedule | ||
cargo run -p mdbook-course --bin course-schedule > upstream-schedule | ||
|
||
- name: "Download artifact from PR workflow" |
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.
Nit: I think this can be simplified by using download-artifact action. When I was implementing my workflow, I wasn't aware of this.
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'll give it a try! The linked article seemed to say that wouldn't work, but on a closer reading maybe that's not true. Also, it's quite an old article so maybe it works now :)
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 haven't tried it, so it might indeed be not working :)
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.
Ah, it requires a PAT with read access to the source repository, and won't accept the repository
input parameter without one -- even if that repository might be public.
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 am ok with not implementing it as well if it is not easy.
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.
(It's possible that the script I've used also won't work with a different, even public, repository -- we'll see!)
await github.rest.issues.createComment({ | ||
owner: context.repo.owner, | ||
repo: context.repo.repo, | ||
issue_number: context.payload.workflow_run.pull_requests[0].number, |
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.
This might be flaky as I discovered. In mdbook-i18n-helpers
I fixed it by google/mdbook-i18n-helpers#168
According to my testing, the root cause of the issue is that context.payload.workflow_run.pull_requests
array is not always populated. Some discussion on this issue https://github.com/orgs/community/discussions/25220
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.
Ugh, GH actions really are ugly. I wasn't able to test on a forked repository since I didn't have a place to fork from that would support running actions.
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.
Yes, debugging them is very cumbersome. It is possible to create a PR against your own fork though. That's what I did in my fork https://github.com/kdarkhan/mdbook-i18n-helpers/pulls?q=is%3Apr+is%3Aclosed
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.
Yeah, that's how I was testing -- but I think it doesn't accurately simulate the permissions of a cross-owner PR.
I don't have approval permissions for this repo but LGTM. |
Let's fix this, I've invited you now! |
This