Why does this repository have two identical github workflows?

[angelikatyborska]

There are two almost identical workflows in this repository, ci.yml and pr.ci.yml. Research and document:

• Why are there two different ones?
• What are their differences?
• Are some of the current differences unintentional? It's easy to forget to edit the second one.
• Can we actually get rid of one of them? Will we lose anything?

[dabaer]

It looks like the checks are relaxed when committing directly to the repository, and some differences in the CI cache.

• pr-check.sh checks concepts and exercise order (on top of what ci-check.sh checks already)
• pr.ci for some reason computes the cache key twice in different steps
• ci uses a different cache key for Retrieve Mix Dependencies Cache in the final step, and skips redundant compute step

Otherwise the files are line for line save for some names (PR vs Main CI, on push vs on pr), and pr.sh and ci.sh are identical.

What do you think, the extra two concept/exercise checks could be merged into ci-check.sh, and the cache values in ci.yml seem fine?

Looking at the history, they were even more similar at the beginning. The cache key computation was the same, and the *-check.sh files were identical. The calls to pr.sh and ci.sh were different initially, but were unified at some point.

I'm guessing there was a plan to have separate checks for maintainers and prs that never came to fruition. I don't see anything being lost if they are merged, and maintainers will receive extra checks when committing directly to a branch.

[dabaer]

diff --git a/.github/workflows/ci.yml b/.github/workflows/pr.ci.yml
index e0990168..dbf4039c 100644
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/pr.ci.yml
@@ -1,15 +1,12 @@
 # This workflow will do a clean install of the dependencies and run tests across different versions
 #
 # Requires scripts:
-# - bin/ci.sh
-# - bin/ci-check.sh
+# - bin/pr-check.sh
+# - bin/pr.sh

-name: elixir / main ci
+name: elixir / pr

-on:
-  push:
-    branches: [master, main]
-  workflow_dispatch:
+on: pull_request

 jobs:
   precheck:
@@ -20,7 +17,7 @@ jobs:
         otp: [26.0]

     steps:
-      - name: Checkout code
+      - name: Checkout PR
         uses: actions/checkout@v3

       - name: Use Elixir
@@ -66,8 +63,8 @@ jobs:
           mkdir -p priv/plts
           mix dialyzer --plt

-      - name: Run Prechecks
-        run: bin/ci-check.sh
+      - name: Run exercism/elixir ci pre-check (stub files, config integrity)
+        run: bin/pr-check.sh

   ci:
     runs-on: ubuntu-20.04
@@ -98,12 +95,22 @@ jobs:
           otp-version: ${{matrix.otp}}
           elixir-version: ${{matrix.elixir}}

+      - name: Set cache key
+        id: set_cache_key
+        run: |
+          erl -eval '{ok, Version} = file:read_file(filename:join([code:root_dir(), "releases", erlang:system_info(otp_release), "OTP_VERSION"])), io:fwrite(Version), halt().' -noshell > ERLANG_VERSION
+          cat ERLANG_VERSION
+          elixir --version | tail -n 1 > ELIXIR_VERSION
+          cat ELIXIR_VERSION
+          cache_key="os-${{ runner.os }}-erlang-$( sha256sum ERLANG_VERSION | cut -d ' ' -f 1 )-elixir-$( sha256sum ELIXIR_VERSION | cut -d ' ' -f 1 )-mix-lock-${{ hashFiles(format('{0}{1}', github.workspace, '/mix.lock')) }}"
+          echo "::set-output name=cache_key::$cache_key"
+
       - name: Retrieve Mix Dependencies Cache
         uses: actions/cache@v3
         id: mix-cache # id to use in retrieve action
         with:
           path: deps
-          key: ${{ runner.os }}-${{ matrix.otp }}-${{ matrix.elixir }}-mix-${{ hashFiles(format('{0}{1}', github.workspace, '/mix.lock')) }}-v2
+          key: mix-${{ steps.set_cache_key.outputs.cache_key }}-v1

       - name: Install Mix Dependencies
         if: steps.mix-cache.outputs.cache-hit != 'true'
@@ -112,5 +119,5 @@ jobs:
       - name: Build Project
         run: mix

-      - name: Run Checks
-        run: bin/ci.sh
+      - name: Run exercism/elixir ci (runs tests)
+        run: bin/pr.sh

[angelikatyborska]

Hmm. That is a very small difference between the two workflows. I think they just should be merged.

Pushing directly to main is not allowed, so every change will go through the GitHub PR process. Still, I think both workflows need to be the same.

If I understand correctly, the PR workflow will run when a PR is open and then on every new commit to the PR. The workflow runs against the code in the PR. Then the PR waits for approval and merging. In that time, changes to main might happen that do not trigger git conflicts (do not force you to edit the PR), but will cause the workflow to fail. For example - two people adding two exercises in two different PRs with the same id. So I think for technical reasons, the CI workflow (run on commits to main) only needs to run checks that can trigger errors due to parallel changes like that (not causing conflicts). However, in practice, it is just much more sane to always run exactly the same code quality checks in both cases. We might lose some time waiting longer for the workflows to finish, but we won't need to deal with forgetting to make a change in one of the workflows but not the other 🤔

[dabaer]

Oh okay, so basically the main-ci workflow runs when you commit to a branch other than main right? So that would be checked before being PR'd into main. Isn't that a redundant check?

[angelikatyborska]

I'm not sure I understand the question correctly. If you're asking about the hypothetical ideal solution, then we want a single CI workflow that runs on every commit on every PR and on every commit to main. This is necessary to ensure all PRs pass the checks, and that main still passes all the checks after merging two or more PRs in parallel that passed the checks separately, but might still cause a failure when applied together.

"Running on every PR" basically means running on every commit on a branch that has a PR open, as far as I understand.

[dabaer]

Ah yup, that explanation made it click for me. It's not the direct commit to a branch but the merge into main itself that gets checked.

So yeah, all of what applied before and also add the clause to run on both PR and commit which is possible in one unified file should do the trick

on:
  push:
    branches:
      - main
  pull_request:

We could also remove the master branch filter as I don't see a reason where that would need to apply in this repo.