cloudflare / pages-action

MIT License
468 stars 95 forks source link

Create GitHub deployment using correct branch name #70

Closed aaronadamsCA closed 1 year ago

aaronadamsCA commented 1 year ago

This replaces #56 and re-fixes #22/#39.

56 was reverted because it turned out some users enable GitHub deployments and use fake branch names when deploying to Cloudflare Pages. When #56 was released, those users began to experience errors (#67).

This PR takes a simpler approach, ignoring user input and using the real GitHub branch name.

WalshyDev commented 1 year ago

I really really appreciate you making the PR, seeing there was an issue, finding the info and then fixing it too. Thank you so much!

I'll review this shortly :)

jonatansberg commented 1 year ago

I tried out @aaronadamsCA fix in our pull_request workflow and it does indeed fix the issue with deployments not being associated with the correct environments in GitHub. Would love to see this merged soon!

aaronadamsCA commented 1 year ago

Oh yeah, I ended up having a quick discussion with GitHub support about this:

When a pull request is open and there are no merge conflicts, GitHub creates a branch with the format refs/pull/{pr_number}/merge. This is the branch that GitHub is checking out by default when you use actions/checkout to check out your code. It represents the test merge branch for the pull request. This branch is a valid branch while the pull request is open and you can create deployments targeting it - GitHub isn't making any assumptions about how you want to create your deployments.

So right now pull_request events create GitHub deployments that reference the temporary PR merge branch. With this change they'll instead reference the PR head branch, which will cause them to appear on the PR as expected.

As for why deployments that target a PR merge branch don't appear on that PR: "🤷‍♂️"

ferferga commented 1 year ago

@aaronadamsCA Please, allow this through an input, so we can specify the branch name if we wish to.

We're not using the pull_request event for deployment since it could expose secrets on forks: We have a Deploy workflow that triggers when a workflow_run has been completed and gets the PR context from the artifacts produced by the PR's workflow

aaronadamsCA commented 1 year ago

@ferferga This PR is strictly a bugfix and does not introduce any new features. I would suggest opening a feature request.

aaronadamsCA commented 1 year ago

@ferferga The problem with your suggestion is that it just causes #67 again. Those users, who pass branch values that do not correspond to real GitHub branch names, will just get "No ref found" again.

To get the behaviour you're looking for, you should probably file a feature request for yet another input, probably named githubBranch to differentiate from the Cloudflare branch input. My mistake in #56 was to conflate the two, which I won't do again here because it will just lead to another reversion.

CC @WalshyDev to confirm you agree (with the limitation on this PR, not with the possible new feature).

WalshyDev commented 1 year ago

Yep exactly, the branch input is still used for the actual Pages deployment but it can't be used for the GitHub deployment (since they require a real branch)

ferferga commented 1 year ago

@aaronadamsCA Oh great! I understood this properly now! Thank you for your insight!

Will do another round check to see if I'm missing something in my workflows and then open a feature request if needed.

amacneil commented 1 year ago

Just spent about 2 hours trying to figure out why deployment links weren't showing up on my PR, then realized this code I was reading on main hasn't been released yet.

Any chance we could get a new release with these changes included?

WalshyDev commented 1 year ago

Planning on a release after merging #82

amacneil commented 1 year ago

Awesome. While I have you, what do you think about making branch default to githubBranch (if not specified) in a future version?

Because Github Actions checks out the PR merge commit by default (with a "detached head"), wrangler doesn't detect the branch name, so if I want the correct branch name to appear in Cloudflare Pages UI I have to pass it explicitly, copying the same logic from this PR:

- uses: cloudflare/pages-action@09ef98de2f7109541029b235a5dc1e40c80d0400
  with:
    accountId: ${{ secrets.CLOUDFLARE_ACCOUNT_ID }}
    apiToken: ${{ secrets.CLOUDFLARE_API_TOKEN }}
    gitHubToken: ${{ github.token }}
    branch: ${{ github.head_ref || github.ref_name }}
    projectName: foo

It would be easier if branch defaulted to those settings. It would also be easier if gitHubToken defaulted to the built in GitHub token so we don't need to pass either setting explicitly.