cloudflare / pages-action

MIT License
468 stars 95 forks source link

Use branch input when determining GitHub deployment production status #80

Closed iiroj closed 1 year ago

iiroj commented 1 year ago

This PR fixes issue https://github.com/cloudflare/pages-action/issues/79.

iiroj commented 1 year ago

You can see this PR in action (pun intended) here: https://github.com/iiroj/iiro.fi/actions/runs/4777207087

It succesfully created the Production environment deployment as can be seen here: https://github.com/iiroj/iiro.fi/deployments/activity_log?environment=iiro+%28Production%29

iiroj commented 1 year ago

Actually, looking a bit deeper into the Cloudflare Pages API documentation, the deployment response contains info about the environment, so this could be used directly to set the GitHub Deployment as well. What do you think?

https://developers.cloudflare.com/api/operations/pages-deployment-create-deployment

WalshyDev commented 1 year ago

This was done originally however, we have a lot of users specifying branch names that do not exist on GitHub. Deployments require a branch which does exist. See https://github.com/cloudflare/pages-action/pull/70 for more context.

iiroj commented 1 year ago

Thanks for the reply, @WalshyDev. How about implementing this as an optional input then, for example gitHubDeploymentBranch?

WalshyDev commented 1 year ago

If this is just to detect the prod env properly for GitHub deployments I think a better solution is to change the prod check.

- const productionEnvironment = githubBranch === project.production_branch;
+ const productionEnvironment = githubBranch === project.production_branch
+  || branch === project.production_branch;

I don't think another branch input is a good idea here. We have tons of inputs now, we need to make sure they're controlled and not confusing.

iiroj commented 1 year ago

@WalshyDev thanks, your comment made me realise I was thinking this in a bit too complicated way. I dropped one of the commits, and now the GitHub Deployment Preview/Production status is selected based only on the production status of the created Cloudflare Pages deployment.

This still works for my use-case where the production deployment is created from a semver tag (by specifying branch: main input to the action).

What do you think?

WalshyDev commented 1 year ago

I like this approach. What this is now doing though is making it so Deployments only have 1 stage, deployed/failed. Previously, we had 2 stages. Deploying and deployed/failed.

Since the actual deployment has moved up, we kinda act like there's 2 stages but it'll only go into the deploying state after it's already done.

https://github.com/cloudflare/pages-action/pull/80/files#diff-a2a171449d862fe29692ce031981047d7ab755ae7f84c707aef80701b3ea0c80R142

https://github.com/cloudflare/pages-action/pull/80/files#diff-a2a171449d862fe29692ce031981047d7ab755ae7f84c707aef80701b3ea0c80R153-R164

iiroj commented 1 year ago

True, I can verify it in my own Actions logs. I will investigate if it can be worked around, and otherwise just use "duplicate" logic like you suggested with branch === project.production_branch