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

Run on pull requests #73

Open
wants to merge 32 commits into
base: main
Choose a base branch
from
Open

Run on pull requests #73

wants to merge 32 commits into from

Conversation

Tom-van-Woudenberg
Copy link
Member

No description provided.

@Tom-van-Woudenberg Tom-van-Woudenberg self-assigned this Jan 23, 2025
@Tom-van-Woudenberg
Copy link
Member Author

It more or less works: a workflow is triggered for pull requests and it builds the book successfully. However, when the commit is done on the forked repo, it seems the workflow doesn't have the authorization to update the github pages website on the original repository, see https://github.com/Tom-van-Woudenberg/jason_test/actions/runs/12925653497/job/36047161353#step:14:32
Whenever the workflow is triggered from the original repo itself, it works: https://github.com/Tom-van-Woudenberg/jason_test/actions/runs/12925727387
@moorepants, you've any ideas?

@moorepants
Copy link

Most people use something like netlify to host temporary viewable HTML outputs from CI builds.

I am not sure if a PR author can have the permissions to update the primary repo's github pages. That just may not be possible, that would mean that anyone could spam your websites with their PRs.

@Tom-van-Woudenberg
Copy link
Member Author

Most people use something like netlify to host temporary viewable HTML outputs from CI builds.

We use GitHub pages for all html outputs, temporary or permanent :)

I am not sure if a PR author can have the permissions to update the primary repo's github pages. That just may not be possible, that would mean that anyone could spam your websites with their PRs.

Yeah makes sense, although it would be desired in our case. I'll add something to the action summary indicating that a run from the original repo is required too.

1 similar comment
@Tom-van-Woudenberg
Copy link
Member Author

Most people use something like netlify to host temporary viewable HTML outputs from CI builds.

We use GitHub pages for all html outputs, temporary or permanent :)

I am not sure if a PR author can have the permissions to update the primary repo's github pages. That just may not be possible, that would mean that anyone could spam your websites with their PRs.

Yeah makes sense, although it would be desired in our case. I'll add something to the action summary indicating that a run from the original repo is required too.

@moorepants
Copy link

We use GitHub pages for all html outputs, temporary or permanent :)

I don't think the PR builds -> pages are possible then.

@Tom-van-Woudenberg
Copy link
Member Author

I don't think the PR builds -> pages are possible then.

Okay, I'll rethink about whether this PR build functionality has any value without the possibility of the 'fork' people to initiate it... The workflow can run in the fork itself too if a preview is needed

@moorepants
Copy link

There is value, which is seeing if the build completes and if not you get all the error messages. That's all I was after when I suggested it. Trying to deploy to temp websites, is an extra feature I suppose. You could just disable the step that tries to push the website content to a new page and then anyone who makes a PR still gets lots of useful info.

@Tom-van-Woudenberg
Copy link
Member Author

@moorepants, I think that's what I've implemented now. Summaries include errors (during build, not before):
Disabled build (shown in summary) when trigger from fork: https://github.com/Tom-van-Woudenberg/jason_test/actions/runs/12930278802
Enabled build (shown in summary) when trigger from not-fork: https://github.com/Tom-van-Woudenberg/jason_test/actions/runs/12930284854

Fancy reviewing this?

@Tom-van-Woudenberg Tom-van-Woudenberg marked this pull request as ready for review January 23, 2025 13:41
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