-
Notifications
You must be signed in to change notification settings - Fork 92
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
Create GitHub deployment using correct branch name #70
Create GitHub deployment using correct branch name #70
Conversation
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 :) |
cloudflare/pages-action, which previously did not work for us on `pull_request` GitHub workflow triggers, now works with @v1.4.0. However, this version has been reverted upstream as of @v1.4.1 (which we were previously using), because, ironically, it broke others' pull request-based workflows. Upstream is now working on a version that will hopefully work for all parties: cloudflare/pages-action#70 In any case, we now split our frontend deployment GitHub workflows into 2: one for pull requests, and one for pushes to `main`. Previously, we had to build & deploy the frontend on *any* push event, which created too many jobs and occasionally blew up in weird ways for pull requests. Note that we could do some DRY here, but a) GitHub makes that remarkably non-ergonomic, and b) I think it's quite likely that we will drop the frontend deployment on pushes to `main` soon, anyway, as that should really be managed now in the `hackworth-code-gitops` repo, instead.
cloudflare/pages-action, which previously did not work for us on `pull_request` GitHub workflow triggers, now works with @v1.4.0. However, this version has been reverted upstream as of @v1.4.1 (which we were previously using), because, ironically, it broke others' pull request-based workflows. Upstream is now working on a version that will hopefully work for all parties: cloudflare/pages-action#70 In any case, we now split our frontend deployment GitHub workflows into 2: one for pull requests, and one for pushes to `main`. Previously, we had to build & deploy the frontend on *any* push event, which created too many jobs and occasionally blew up in weird ways for pull requests. Note that we could do some DRY here, but a) GitHub makes that remarkably non-ergonomic, and b) I think it's quite likely that we will drop the frontend deployment on pushes to `main` soon, anyway, as that should really be managed now in the `hackworth-code-gitops` repo, instead.
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! |
Oh yeah, I ended up having a quick discussion with GitHub support about this:
So right now As for why deployments that target a PR merge branch don't appear on that PR: "🤷♂️" |
@aaronadamsCA Please, allow this through an input, so we can specify the branch name if we wish to. We're not using the |
@ferferga This PR is strictly a bugfix and does not introduce any new features. I would suggest opening a feature request. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aaronadamsCA The original fix allowed to introduce manually the branch, so I saw it fitting here, so imo it's a regression😅.
Looking at the code, the branch
input doesn't have a purpose now either?
What I suggest is as simple as this, so I don't see why it couldn't be done here either when it was possible before as I said.
@ferferga The problem with your suggestion is that it just causes #67 again. Those users, who pass To get the behaviour you're looking for, you should probably file a feature request for yet another input, probably named CC @WalshyDev to confirm you agree (with the limitation on this PR, not with the possible new feature). |
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) |
@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. |
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 Any chance we could get a new release with these changes included? |
Planning on a release after merging #82 |
Awesome. While I have you, what do you think about making 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 |
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.