8000 Add PR Previews by michaelwalshe · Pull Request #406 · PSIAIMS/CAMIS · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Add PR Previews #406

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Apr 15, 2025
Merged

Add PR Previews #406

merged 4 commits into from
Apr 15, 2025

Conversation

michaelwalshe
Copy link
Contributor
@michaelwalshe michaelwalshe commented Apr 11, 2025 •
edited
Loading

This PR updates the Pull Request and Deployment actions to allow for previews of the PR deployments.

It uses the action https://github.com/marketplace/actions/deploy-pr-preview, which I've called at the end of .github/workflows/pull_request_action.yml. This will upload the _site directory from the PR rendering output to a new /pr-preview/pull-<n> directory of the gh-pages branch, and create a sticky comment in the PR that links to this preview section of the site.

There is an example pull request for this at michaelwalshe#1.

A couple of notes about this change:

  • After a PR is closed, the preview will be automatically deleted
  • The PR previews are currently not supported from forks, only from PR's from branches within the repo. This is a limitation, however allowing deploying artifacts to the repo from forks comes with fairly significant security risks (see Parametrise PR values rossjrw/pr-preview-action#6). We could add support for this, by customising the action, however would increase maintenance (makes the action more complicated for us) and have the aforementioned security risks.
  • This implementation adds the previews to the main site, under a pr-preview subfolder. This won't be visible from the main site unless someone knows where to look (it isn't linked to). We could alternatively setup a second CAMIS-preview repository, with a separate github pages deployment, that the PR previews are uploaded to, however that again comes with some additional maintenance required.
  • There is a slight delay between the PR rendering finishing and the comment on the PR being created, and the page actually being visible. This is only a minute or so, but could confuse some contributors.

Happy to discuss further, this is only a sample implementation!

statasaurus
statasaurus approved these changes Apr 14, 2025
View reviewed changes
Copy link
Contributor
@statasaurus statasaurus left a comment

Choose a reason for hiding this comment

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

This looks amazing! Do you think it is ready to be pulled in? I am not totally sure we need to add if about excluding forks. At this point a good chunk of our PR's are from people outside the repo and this guidance leaves me to believe that forks will need approval to run anyway

@michaelwalshe
Copy link
Contributor Author

@statasaurus We can set it up so that it runs on forks, but the logic in the yaml is just because it will error at the moment as the pr preview action itself doesn't support running on forks (see this build)

I can look into setting this up so it won't works on PRs from forks if you're happy with the security risk. As you've said, we can just set it so that PRs from forks require approval to run, currently I think the default is that approval is needed the first time but not after that, may want to change it so it's a little stricter?

That will mean this change takes longer to finish, as I'll have to write a new version of the action, and we'll have to do some faffing with gh tokens. It may be easiest to merge this change as the next best thing, then I can look at extending the functionality to forks in the future?

@michaelwalshe michaelwalshe marked this pull request as ready for review April 14, 2025 16:08
@statasaurus statasaurus merged commit 750e581 into PSIAIMS:main Apr 15, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@statasaurus statasaurus statasaurus approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@michaelwalshe @statasaurus
0