flybywiresim / docs

https://docs.flybywiresim.com/
36 stars 79 forks source link

feat: Adding PR Build and Deploy actions for all PRs #1056

Closed pdellaert closed 1 week ago

pdellaert commented 1 week ago

Summary

Uses https://github.com/marketplace/actions/refined-cloudflare-pages-action#enabling-pr-previews-from-forks for building and deploying PR Previews to Cloudflare pages.

Setup

Requires a few new secrets:

On Cloudflare Pages, it is best to only allow automatic building of the main branch (primary) of the main repo, and not other branches, although this is not a must. If building all branches remains enabled, if a PR is created for a branch on the main repo, two deployments will exist for the PR. One for the branch, and one for the PR itself (using this change).

Functionality

This is a two-stepped approach so that PRs do not need access to secrets (fork PRs have no access to secrets of the main repo):

  1. PR Build & Upload - This builds and uploads an archive of the build locally for the PR itself. This is triggered by opening or syncing a PR (pushing to it)
  2. PR Deploy - This runs after the PR Build, and runs inside the secure context of the main repo, with no visibility into the secrets by the PR or PR Repo. It takes the archive and creates a Preview deployment for the PR (name is <repo-owner>-<branch>.<project>.pages.dev.
  3. The action creates or updates (if new commits) a comment on the PR with the Preview deployment link after build

Proof of concept

Both have been deployed on CF Pages as previews, with multiple commits (create PR, and adding more commits after): image Note that in this test deployment, only Primary (production) branch is enabled for automatic build. For other branches on the main repo, the automatic builds are disabled.

github-actions[bot] commented 1 week ago

This PR is being prevented from merging because Review Required. Use the Approved label to run build validation and auto merge the PR.

Valastiri commented 1 week ago

Noted will review. Small point in regards to the following:

On Cloudflare Pages, it is best to only allow automatic building of the main branch (primary) of the main repo, and not other branches, although this is not a must. If building all branches remains enabled, if a PR is created for a branch on the main repo, two deployments will exist for the PR. One for the branch, and one for the PR itself (using this change).

Quick first question is does this change overwrite the current functionality of having a preview build for any feature branch (on main repo) that has a PR to primary? Bit confused apologies.

If no we don't really need to build other branches unless they are PR'd since they are just technically working feature branches. I know that vercel did this and allowed us to technically preview feature branches that did not have a PR but we never really needed to use this feature.


Second question - with the current iteration of this PR if I created a PR as previously for say multi-aircraft-restructure which had the following:

Would the second bullet point still generate a PR preview?

pdellaert commented 1 week ago

Noted will review. Small point in regards to the following:

On Cloudflare Pages, it is best to only allow automatic building of the main branch (primary) of the main repo, and not other branches, although this is not a must. If building all branches remains enabled, if a PR is created for a branch on the main repo, two deployments will exist for the PR. One for the branch, and one for the PR itself (using this change).

Quick first question is does this change overwrite the current functionality of having a preview build for any feature branch (on main repo) that has a PR to primary? Bit confused apologies.

If no we don't really need to build other branches unless they are PR'd since they are just technically working feature branches. I know that vercel did this and allowed us to technically preview feature branches that did not have a PR but we never really needed to use this feature.

@Valastiri The behavior when disabling automatic builds in CF itself, in combo with this PR, is such: If a (feature) branch is created on the main repo, no preview is build and deployed until a PR is opened for that branch to be merged in another branch. Nothing stops us from enabling automatic build for all (feature) branches, if then a PR is opened for the feature branch, it would be deployed twice: Once by CF automatically for the branch; Once for the PR by the new workflows.

The initial suggestion from the custom action we are using here, is to completely disable Cloudflare Github direct integration. Which means no CF Automatic builds on Production (Primary) branch. If we do that, we would also need to create a similar workflow for changes on the primary branch (and optionally one for feature branches).

But I find CF building the primary branch automatically is probably a nice thing. And have PRs be managed by these workflows.

Second question - with the current iteration of this PR if I created a PR as previously for say multi-aircraft-restructure which had the following:

  • multi-aircraft-restructure had a PR -> primary
  • There were multiple working PRs against multi-aircraft-restructure

Would the second bullet point still generate a PR preview?

Yep, any PR would generate a preview.