GHA workflow to avoid PR conflicts from local-built dist files

[tombogle]

This change is 

[lyonsil]

I have a couple of concerns here:

1. This workflow sort of trades one problem for another. Ira was just pointing out that our build job times are going up significantly already. This will make that problem worse.
2. This workflow creates the risk that we'll end up with out-of-date dist files if there is any problem with the "follow up workflow."

If the team is OK with these risks, then I think this change makes sense (assuming it actually works as expected! 😆 ). It's probably worth discussing in a sprint retrospective or something similar.

Note that people will still have to build PBU/PBR locally when they are doing development. The main change here is not having to worry about conflicts with the dist files in PRs since the dist files will be rebuilt automatically.

@lyonsil reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions

---

.github/workflows/test.yml line 134 at r1 (raw file):

        run: npm run build:platform-bible-react

      - name: Build platform-bible-utils

If we go forward with this, PBU needs to be build first since PBR relies on it. Same with the other YML file.

---

.github/workflows/auto-build-and-approve-dist.yml line 35 at r1 (raw file):

        run: npm run build:platform-bible-utils

      # Check if only dist folders changed

I might be misreading things, but I think this Check if only dist folder changed section is no longer necessary since this workflow should only trigger if something other than dist files changed in the libs. If I'm right, then the if checks in the next two sections are also not necessary.

[tombogle]

1. Yes. I tried to come up with a simple and reliable way to only have it rebuild the libraries if a relevant source file changed, but after working through several possible solutions, it just started feeling really iffy and error-prone, and I was reticent to pursue it further unless/until we a) know it works at all, and b) know that the effort is warranted.
2. For sure, we'd need to test it out and do it on a trial basis. We can always revert back to the old way if the cure proves worse than the disease.
   It's true that it will still generally be necessary to build locally For sure if the changes are done as part of a PR that requires them in core, and even if not, probably still a good idea to make sure nothing got broken.

[tombogle]

Reviewable status: 1 of 2 files reviewed, 2 unresolved discussions (waiting on @lyonsil)

---

.github/workflows/auto-build-and-approve-dist.yml line 35 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

I might be misreading things, but I think this Check if only dist folder changed section is no longer necessary since this workflow should only trigger if something other than dist files changed in the libs. If I'm right, then the if checks in the next two sections are also not necessary.

I think the point of this is a sanity check. Only dist files should have changed as a result of these builds, so if something else did change, there's a problem and we shouldn't roll forward.

---

.github/workflows/test.yml line 134 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

If we go forward with this, PBU needs to be build first since PBR relies on it. Same with the other YML file.

Fixed

[tjcouch-sil]

I have the same concerns Matt does. If the team prefers to try this, that's fine. Thanks for putting something together.

@tjcouch-sil reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @lyonsil)

---

.github/workflows/auto-build-and-approve-dist.yml line 62 at r2 (raw file):

        if: steps.check-dist.outputs.dist_only == 'true'
        run: |
          git commit -m "chore: update dist [skip ci]"

Why does this say "skip ci"? I can't tell what it means in this context.

---

.github/workflows/auto-build-and-approve-dist.yml line 66 at r2 (raw file):

      # Auto-approve PR if only dist changed
      - name: Auto-approve dist-only PRs

How do we intend to handle Reviewable? I suspect it will block the PR, so this may not be as automatic as we hope.

---

.github/workflows/test.yml line 131 at r2 (raw file):

      # Ensure the `dist` files are updated before running tests.
      - name: Build platform-bible-utils

Won't we have this same problem with any workflow that relies on these files? I think of the package workflow, for example.

[tombogle]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @lyonsil and @tjcouch-sil)

---

.github/workflows/auto-build-and-approve-dist.yml line 62 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Why does this say "skip ci"? I can't tell what it means in this context.

[skip ci] is a special marker in commit messages recognized by most CI providers (including GHA) that tells them not to run any workflows as a result of that commit. That is, it affects what happens after the commit is pushed. Since we already know the build passed, and this commit just contains output from it, there is probably no need to trigger another full CI pipeline. In some scenarios, this could cause infinite loops, though in our case, I think it would be harmless (but unnecessary).

---

.github/workflows/auto-build-and-approve-dist.yml line 66 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

How do we intend to handle Reviewable? I suspect it will block the PR, so this may not be as automatic as we hope.

I think it will work, but we won't know for sure until we try it. The GitHub Action submits an “Approve” review. From what I understand, the branch protection rules don't know or care whether the approval came through Reviewable or directly through GH. Unless I'm wrong about that, we should be fine. (Someone could test this by approving this PR through GH. If it then becomes eligible to merge, we'll know this should work. We won't actually merge it until we agree to give it a try, though we probably won't know if we actually like it until we try it.)

[tjcouch-sil]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @lyonsil and @tombogle)

---

.github/workflows/auto-build-and-approve-dist.yml line 66 at r2 (raw file):

Previously, tombogle (Tom Bogle) wrote…

I think it will work, but we won't know for sure until we try it. The GitHub Action submits an “Approve” review. From what I understand, the branch protection rules don't know or care whether the approval came through Reviewable or directly through GH. Unless I'm wrong about that, we should be fine. (Someone could test this by approving this PR through GH. If it then becomes eligible to merge, we'll know this should work. We won't actually merge it until we agree to give it a try, though we probably won't know if we actually like it until we try it.)

My understanding is that Reviewable is a required check that blocks PRs :/

We could make it not required, but that doesn't seem like a great solution

[tombogle]

.github/workflows/auto-build-and-approve-dist.yml line 66 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

My understanding is that Reviewable is a required check that blocks PRs :/

We could make it not required, but that doesn't seem like a great solution

After further research, I'm almost 100% sure that does not mean that it has to be approved via the Reviewable UI. By virtue of having Reviewable set up for the project, it hooks into the GH UI so that the pending-approval link opens Reviewable instead of the normal GH UI. I know in my other projects that require approvals and display a link to reviewable there, the approval can also be done using conventional GH UI, and then GH is totally happy to allow the merge. (I think you can even then do the merge via the Reviewable UI.) I looked in the GH Branch Protection Rules for the HearThis project, and all you can do is require a certain number of approvals, but there is nowhere in those settings where you can require that the approval come through Reviewable. Mr. AI agrees: "You can't force users to only use Reviewable via a GitHub setting."

[tjcouch-sil]

Another concern I have is that this would presumably mean we would no longer see index.d.ts changes in PRs. Seeing the changes in that file is very valuable to me for seeing what API changes are happening and considering if those API changes are worth making, need TSDocs, etc. It's one thing to hunt this kind of stuff down in the .ts files and hope you're interpreting it right, but it's another to see the final type output actually have everything that is changing.

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @lyonsil and @tombogle)

---

.github/workflows/auto-build-and-approve-dist.yml line 66 at r2 (raw file):

Previously, tombogle (Tom Bogle) wrote…

After further research, I'm almost 100% sure that does not mean that it has to be approved via the Reviewable UI. By virtue of having Reviewable set up for the project, it hooks into the GH UI so that the pending-approval link opens Reviewable instead of the normal GH UI. I know in my other projects that require approvals and display a link to reviewable there, the approval can also be done using conventional GH UI, and then GH is totally happy to allow the merge. (I think you can even then do the merge via the Reviewable UI.) I looked in the GH Branch Protection Rules for the HearThis project, and all you can do is require a certain number of approvals, but there is nowhere in those settings where you can require that the approval come through Reviewable. Mr. AI agrees: "You can't force users to only use Reviewable via a GitHub setting."

This setting seems to indicate it is required:

[lyonsil]

@lyonsil reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @tombogle)

---

.github/workflows/auto-build-and-approve-dist.yml line 35 at r1 (raw file):

Previously, tombogle (Tom Bogle) wrote…

I think the point of this is a sanity check. Only dist files should have changed as a result of these builds, so if something else did change, there's a problem and we shouldn't roll forward.

Fair enough - makes sense

[tombogle]

The build could automatically generate a diff of index.d.ts vs. the base branch as an artifact. Would that meet your need?

Reviewable status: 1 of 2 files reviewed, 2 unresolved discussions (waiting on @lyonsil and @tjcouch-sil)

---

.github/workflows/auto-build-and-approve-dist.yml line 66 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

This setting seems to indicate it is required:

So it is. Then I think the only viable way forward (that wouldn't require manual intervention) would be to not require code-review/reviewable for bot-created, dist-only PRs. We could create a GitHub bot account that would create these PRs, and in branch protection settings, exempt that bot account from this required check. Would we be comfortable with that?

---

.github/workflows/test.yml line 131 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Won't we have this same problem with any workflow that relies on these files? I think of the package workflow, for example.

Yes :-(. I think this is still doable with the right rules, but it does mean complicating that workflow (and maybe others???) as well. I won't pursue further until we decide if we're okay with relaxing code-review/reviewable...