rossjrw / pr-preview-action

GitHub Action that deploys a pull request preview to GitHub Pages, similar to Vercel and Netlify, and cleans up after itself.
https://github.com/marketplace/actions/deploy-pr-preview
MIT License
287 stars 46 forks source link

Support retrieving latest version tag also when pinning action #72

Closed netomi closed 11 months ago

netomi commented 11 months ago

This fixes #67 .

Right now, the action can not be pinned, this PR fixes that so that the referenced tag can also be determined from a commit hash.

rossjrw commented 11 months ago

Removing the --single-branch optimisation: yep, this is pretty non-negotiable if we want to able to version this with a SHA. It's gotta go. Thanks for the contribution!

Changing the tag finding mechanism: at first glance I don't like this new approach using git tag --points-at. I can find a couple of ways to break it already, both of which are pertinent when considering the use case of pinning a specific version, tagged or untagged, in a forked version of the action:

I don't think the git describe --tags solution would cause an issue when a SHA is checked out - might look a bit weird (v1.4.4-4-g2150c59 for current main) but isn't broken. Is there a problem I've missed?

netomi commented 11 months ago

I am open to make this more resilient, the PR so far was my attempt to make it work with a commit hash, but I will invest more time into it.

rossjrw commented 11 months ago

Ah! I think I understand the reason for the change. When you clone the entire bare repository you don't have anything checked out, so git describe doesn't know to describe the SHA you specified.

git describe can take a commit parameter, so I think this can be fixed by changing:

action_version=$(git describe --tags --match "v*.*.*" || git describe --tags || git rev-parse HEAD)

to

action_version=$(git describe --tags --match "v*.*.*" "$git_ref" || git describe --tags "$git_ref" || git rev-parse HEAD)

Does that seem reasonable to you?

netomi commented 11 months ago

yes that is exactly the reason for the change, but I will test if git describe with the ref leads to the same result as my snippet.

netomi commented 11 months ago

ok tested that with various refs and the result is basically the same, so that looks reasonable to me, will update the PR.

netomi commented 11 months ago

updated the PR.

btw. you might wanna look at https://github.com/eclipse-langium/langium-website/blob/main/.github/workflows/preview.yml to support that the comments can also be added to the PR for PRs coming from forks.

It uses the pull_request_target trigger to be able to issue write tokens and deploy the site to a separate repo using a fine-grained token that can only access that repo. Thats a reasonable compromise imho together with approval for workflows running for any PR from outside collaborators.

rossjrw commented 11 months ago

Thanks @netomi, will merge this later today.

That permissions setup does seem like a reasonable compromise. Will be worth me documenting it as a possible solution for #3

rossjrw commented 11 months ago

Released in v1.4.6

netomi commented 11 months ago

That permissions setup does seem like a reasonable compromise. Will be worth me documenting it as a possible solution for #3

I looked at the issue and the proposed solution with workflow_call which I believe is technically sound, however there is still the problem where to push the built preview site to for deployment. Pushing it to the same repo requires write permissions for content and I feel uneasy to grant that to any workflow that would run for PRs coming from forks. So the solution with a dedicated preview repo is a solution to that. Maybe doing the comment with a workflow_call would be useful, but I would rather always do the deployment to a separate repo.

fyi. the deploy-pages action is working on a preview feature: https://github.com/actions/deploy-pages/tree/b580d214b4e13b2a70d0e04376a86ed862ebb558#inputs-

which is currently in closed alpha

rossjrw commented 11 months ago

Agreed re deploying to a separate repo. Fundamentally this entire action is a bad idea and I don't think it can ever be truly secure - but it is convenient. I do think that when it gets as far as needing an entire separate repo just for deployments, at that point you might as well use a dedicated deployment provider, which is why I'll recommend solutions like Netlify and Vercel at the drop of a hat (despite never having actually used either myself).

fyi. the deploy-pages action is working on a preview feature: https://github.com/actions/deploy-pages/tree/b580d214b4e13b2a70d0e04376a86ed862ebb558#inputs-

I've seen. There's some discussion about integration with that action here https://github.com/rossjrw/pr-preview-action/issues/21, and about the alpha 'preview' feature specifically in this comment https://github.com/rossjrw/pr-preview-action/issues/21#issuecomment-1824112900

The gist of my opinion is that there's no indication of whether that feature is being worked on, if it will be worked on, or even a development roadmap. Based on https://github.com/actions/deploy-pages/pull/61 and https://github.com/actions/deploy-pages/issues/180 there have been no updates since September 2022, so I have no expectations that the feature will ever be generally available. I'm not going to pin any hopes on it until I hear otherwise.

netomi commented 11 months ago

Indeed, this action is a convenient by simple way to support previews. I have used netlify as well and it works like a charm, but not everybody is willing to use it for different reasons. I am confident that this action will remain useful regardless on the progress wrt the preview feature in the deploy-pages action.