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
267 stars 43 forks source link

Leverage actions/configure-pages to get pagesurl #26

Closed vincerubinetti closed 1 year ago

vincerubinetti commented 1 year ago

Removes custom logic to obtain the pagesurl variable, and offloads that task to the first-party actions/configure-pages action. Should be a better way to do this as: 1) it is straight from the GitHub authority, 2) requires no user input, totally automatic, and 3) probably more robust to any future changes to Pages url schemas (unlikely but nice to have).

Tested, seems to work.

Note that this also enables Pages if not already enabled.

Supercedes #24

Closes #12

rossjrw commented 1 year ago

I like the look of this a lot. Thanks for the contribution! Will take a closer look at this when I get a chance.

vincerubinetti commented 1 year ago

Note that currently there's actually a security bug that caused them to have to remove the "enablement" functionality:

https://github.com/actions/configure-pages/issues/40

So

- name: Get GitHub Pages url
  id: pages
  uses: actions/configure-pages@v2

Would (hopefully temporarily) need

with:
  - enablement: false

to avoid an error.

Or, if you're worried about this never being resolved (or being resolved then coming back later), you could just use the rest API directly with a read-only operation to lookup the pages url:

https://octokit.github.io/rest.js/v19#repos-get-pages https://docs.github.com/en/rest/pages?apiVersion=2022-11-28#get-a-github-pages-site

- name: Get GitHub Pages url
  id: pages
  uses: actions/github-script@v6
  with:
    script: |
      return (await github.rest.repos.getPages({
        owner: context.repo.owner,
        repo: context.repo.repo,
      })).html_url

# then read ${{ steps.pages.outputs.result }}

Up to you which way you want to go. Though it might be nice to have this action automatically enable Pages, since it needs to be enabled for this action to work.

rossjrw commented 1 year ago

Thanks for looking into this so thoroughly, I appreciate it. While I've still not yet had the opportunity to look into this with as much detail as it deserves, I just want to comment on one thing:

it might be nice to have this action automatically enable Pages, since it needs to be enabled for this action to work.

This isn't technically guaranteed to be true - this Action does advertise itself as being solely for Pages previews, but it doesn't have to be that. I can't verify that everyone using this Action is definitely using it only for Pages and nothing else, given that all it does is add files to a given subdir on a given branch.

I'm a little worried that implementing this approach might force the end-user into using Pages. I also admit I'm not convinced that this Action enabling a Pages site automatically is necessarily desirable, given that the target use case is for a repo that already has Pages enabled and if it doesn't it's not really my business to be interfering with that.

The sweet spot for me, I think, would be using configure-pages to get the Pages URL and then if that fails for whatever reason (or if the user has overriden the URL with a parameter), then fall back to old behaviour (maybe sans the org detection stuff).