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

Fix previews workflow #22

Merged
merged 4 commits into from
May 23, 2024
Merged

Fix previews workflow #22

merged 4 commits into from
May 23, 2024

Conversation

brauliorivas
Copy link
Member

BEGINRELEASENOTES

  • PR previews should now work, using the updated preview.yml file.

ENDRELEASENOTES
After taking a closer look, pr-preview-action uses another github action called sticky-pull-request-comment. So I came across issue #930 where happened the same error. So the solution is proposed by the author like this.

@brauliorivas
Copy link
Member Author

Oh It didn't work. I know why now. It "worked" before for @tmadlener because it was a PR inside the same dmX repo (the gh preview worked). However, when PR's are opened from a fork, then It fails, because it doesn't have enough permissions to push the preview code from the fork to the upstream. The author of pr-preview-action suggest to change the type of event, from pull_request to pull_request_target as explained here. Now I have changed it but isn't triggering. I'll try to simulate this with my own org and make a fork of the repo to my own account to test how to get this to work. Also, in theory changing the permissions for PR to write should enable github-actions bot to add the link of the preview as a commentary.

@brauliorivas
Copy link
Member Author

brauliorivas commented May 18, 2024

The new workflow is not triggering but should in theory. I've replicated the same situation using a repo from an organization and a fork. So in PR #7, it couldn't deploy to gh pages because of this error. In dmX happens the same error here

But after changing the trigger to pull_request_target, it worked using the updated workflow file. This PR #22 won't deploy either because it's using the old workflow file, but we can test if this fix finally works for future PR's.

However, there is a drawback. This can raise a security concern because anyone when opening a PR, would push to dmX in the gh-pages branch as explained here. So we would need to run the workflows only under approval (first-time contributors maybe).

@brauliorivas brauliorivas changed the title Add missing permission for preview bot Fix previews May 18, 2024
@brauliorivas brauliorivas changed the title Fix previews Fix previews workflow May 18, 2024
@tmadlener
Copy link
Contributor

Thanks for investigating this and writing everything up so that we have a comprehensive list of things to check when we run into this the next time ;)

Did I understand correctly, that we need to merge this first to see if it actually fixes things?

So we would need to run the workflows only under approval (first-time contributors maybe).

We do that in any case for all our repositories.

@brauliorivas
Copy link
Member Author

Did I understand correctly, that we need to merge this first to see if it actually fixes things?

I guess... I mean, in my example it worked. But maybe some other thing could cause an error. So I would like to give it a try, and if doesn't work, then I will see what happens and try to fix it, again 😅.

We do that in any case for all our repositories.

Ohh, then it's fine.

@tmadlener
Copy link
Contributor

Then let's merge and see what happens ;)

@tmadlener tmadlener merged commit ed395c8 into key4hep:main May 23, 2024
@tmadlener tmadlener mentioned this pull request May 23, 2024
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