RFC: Proposal for GitHub Actions continuous integration workflow

[weierophinney]

We previously voted to migrate to GitHub Actions. Since then, Travis-CI has announced that OSS plans are limited to 10k minutes per month, and we have twice reached the limit by mid-month. On top of that, while we have requested an OSS exemption, and provided proof that we fit their criteria, the last word we had from them was that they were "revisiting the OSS policy".

Since we've now hit the limits a couple times, both Marco and myself have implemented CI via GitHub Actions on the following repositories:

• laminas/laminas-code
• laminas/laminas-cli
• laminas/laminas-eventmanager

At this time, I think I've hit on a workable solution; please see laminas/laminas-eventmanager#15 for details.

To get there:

• Pull the workflow file into the repo (I'll provide wget and/or curl commands in order to simplify this for those doing the work)
• Check the workflow against the current .travis.yml, and:
  • Set an alternate "stable PHP" (for non-unit test checks) in a config file if a version other than 7.4 should be used.
  • Set any php.ini settings in a config file, if needed.
  • Provide a list of additional extensions in a config file, if needed.
  • Provide a list of checks to perform via the config file, if additional or fewer checks are needed.
  • Create any new workflow files necessary if you need to do anything weird (such as test against different extension versions, etc.). This should be rare.
• Remove the .travis.yml (via git rm).
• Check-in the workflow file(s) and configuration file (if needed).
• Create a pull request.

---

Originally proposed solution (obsolete)

The solution uses a combination of the following actions:

• actions/checkout
• actions-cache
• shivammathur/setup-php

We then create one workflow file for each task/tool we want to run. Examples include:

• benchmarks.yml
• cs-checks.yml
• psalm-yml
• unit-tests.yml

A generic workflow file starts like the following:

name: "Name of workflow goes here"

on:
  pull_request:
    paths-ignore:
    - 'mkdocs.yml'
    - '*.md'
  push:
    paths-ignore:
    - 'mkdocs.yml'
    - '*.md'

jobs:
  run:
    runs-on: ubuntu-latest

    steps:
      - name: Checkout
        uses: actions/checkout@v2

      - name: Setup PHP
        uses: shivammathur/setup-php@v2
        with:
          php-version: 7.4
          extensions: mbstring, intl, readline
          ini-values: memory_limit=-1
          coverage: none
          tools: composer:v2, cs2pr

      - name: "Cache dependencies"
        uses: "actions/cache@v2"
        with:
          path: |
            ~/.composer/cache
            vendor
          key: "php-7.4-latest"
          restore-keys: "php-7.4-latest"

      - name: Install dependencies
        env:
          COMPOSER_TOKEN: ${{ secrets.GITHUB_TOKEN }}
        run: |
          composer install --no-interaction --no-progress --no-suggest
          composer show

From there, we add a step for the actual check being performed:

# CS checks
      - name: Run CS checks
        shell: bash
        run: "vendor/bin/phpcs -q --report=checkstyle | cs2pr"
# Benchmarks
      - name: Run benchmarks
        shell: bash
        run: "vendor/bin/phpbench run --revs=2 --iterations=2 --report=aggregate"
# Psalm
      - name: Perform static analysis
        run: "vendor/bin/psalm  --output-format=github --shepherd --stats"

For unit tests, we need a test matrix, and we need to vary things like composer arguments based on PHP version, whether or not to enable coverage, whether or not to upload coverage, etc.. This can be done by creating a matrix strategy, selective definition of matrix variables, and if step conditions and/or conditional environment variable declarations. As a full example:

jobs:
  run:
    runs-on: ubuntu-latest
    strategy:
      fail-fast: false
      matrix:
        php-version:
          - "7.3"
          - "7.4"
          - "8.0"
        deps-version:
          - "lowest"
          - "latest"
        include:
          - php-version: "7.4"
            deps-version: "latest"
            coverage: "xdebug"
          - php-version: "8.0"
            composer-args: "--no-interaction --no-progress --no-suggest --ignore-platform-reqs"

    steps:
      - name: Checkout
        uses: actions/checkout@v2

      - name: Setup PHP
        uses: shivammathur/setup-php@v2
        with:
          php-version: ${{ matrix.php-version }}
          extensions: mbstring, intl, readline
          ini-values: memory_limit=-1
          coverage: ${{ matrix.coverage || 'none' }}
          tools: composer:v2

      - name: "Cache dependencies"
        uses: "actions/cache@v2"
        with:
          path: |
            ~/.composer/cache
            vendor
          key: "php-${{ matrix.php-version }}-${{ matrix.deps-version }}"
          restore-keys: "php-${{ matrix.php-version }}-${{ matrix.deps-version }}"

      - name: Install dependencies
        env:
          COVERAGE_DEPS: 'php-coveralls/php-coveralls'
          DEPS: ${{ matrix.deps-version }}
          COMPOSER_ARGS: ${{ matrix.composer-args || '--no-interaction --no-progress --no-suggest' }}
          COMPOSER_TOKEN: ${{ secrets.GITHUB_TOKEN }}
          TEST_COVERAGE: ${{ matrix.coverage || '' }}
        shell: bash
        run: |
          composer install $COMPOSER_ARGS
          if [[ $DEPS == 'latest' ]]; then composer update $COMPOSER_ARGS ; fi
          if [[ $DEPS == 'lowest' ]]; then composer update --prefer-lowest --prefer-stable $COMPOSER_ARGS ; fi
          if [[ $TEST_COVERAGE != '' ]]; then composer require --dev $COMPOSER_ARGS $COVERAGE_DEPS ; fi
          composer show

      - name: Setup problem matcher for PHPUnit
        run: echo "::add-matcher${{ runner.tool_cache }}/phpunit.json"

      - name: Run unit tests
        env:
          TEST_COVERAGE: ${{ matrix.coverage || '' }}
        shell: bash
        run: |
          if [[ $TEST_COVERAGE != '' ]]; then composer test-coverage ; else composer test ; fi

      - name: Upload coverage
        if: matrix.coverage
        env:
          COVERALLS_REPO_TOKEN: ${{ secrets.GITHUB_TOKEN }}
        run: "vendor/bin/php-coveralls -v"

Things to note in this example:

• The job matching a PHP 7.4 version with latest dependencies sets an additional matrix variable, "coverage", with the value "xdebug".
  • matrix.coverage is queried when determining whether or not to enable coverage in the setup-php action. Further, it is conditionally set to "none" if the value does not exist.
  • matrix.coverage is queried when setting a TEST_COVERAGE env variable when installing dependencies and running tests; it is conditionally set to an empty string if not present.
  • matrix.coverage is used as a conditional for the "Upload coverage" step; this means the step is skipped if the value is not set.
• Jobs matching a PHP 8.0 version set an additional matrix variable "composer-args", with a specific set of arguments to use for those versions.
  • The "Install dependencies" step checks to see if the value is set, and provides a default if not, assigning the value to the COMPOSER_ARGS env variable.
• The GITHUB_TOKEN secret is passed when installing dependencies and uploading coverage, and assigned to the appropriate env variable in each case. This prevents issues with hitting github API limits during installation, and allows us to upload to Coveralls.

A note on setup-php

This action is quite comprehensive in terms of allowing us to add extensions, provide INI settings, and use all the versions of PHP we need to test against; it also provides some tools (such as composer) out-of-the-box.

In the examples above, I am using the default PHP versions provided with the action, which may or may not be the latest versions released. There is a way to force the action to update to the latest version. However, when enabled, the "Setup PHP" step takes 1.5 - 2+ minutes per job (though jobs are run in parallel). When using the defaults, the step only takes 2s.
Since we are typically trying to favor speed of feedback, I argue we do not enable the flag to force updates.

How we get there

My plan is to create default versions of these workflow files, and place them in the repo-template repository. From there, I will create issues on all non-security-only repositories detailing the following:

• Pull the templates into the repo (I'll provide wget and/or curl commands in order to simplify this for those doing the work)
• Check the templates against the current .travis.yml, updating:
  • The test matrix, if needed.
  • The workflow files (to determine which version to run against, and/or whether or not the file is needed — if psalm integration is not present, or no benchmarks are needed, those can be omitted).
  • Additional conditional matrix variables/env variables, as needed.
• Remove the .travis.yml (via git rm).
• Check-in the workflow files.
• Create a pull request.

At that point, we can put out a call-to-action in the Slack and via Twitter to get folks to help, and those of us on the TSC can work on these as well.

[rieschl]

I really appreciate the switch to GH actions!
One thing though.. Some time ago I noticed issues with actions when a non-maintainer opens a PR on such a repo. Specifically (if I remember correctly) there were issues if the action posted a comment in the PR. Afaik the GITHUB_TOKEN is read-only. Is that already resolved by Github? Or maybe not even an issue here (I'm not that familiar with workflow events).

If you want I can open a PR on one of the above repos which adds a failing test and with wrong coding standards and see what happens 🙂

laminas/lamians-cli

I had to chuckle 😉 It's funny that even Mr Laminas himself sometimes mistypes Laminas 🙂

edt: it's GITHUB_TOKEN, not GITHUB_SECRET

[Ocramius]

I really appreciate the switch to GH actions!
One thing though.. Some time ago I noticed issues with actions when a non-maintainer opens a PR on such a repo. Specifically (if I remember correctly) there were issues if the action posted a comment in the PR. Afaik the GITHUB_SECRET is read-only. Is that already resolved by Github? Or maybe not even an issue here (I'm not that familiar with workflow events).

See https://github.blog/2020-08-03-github-actions-improvements-for-fork-and-pull-request-workflows/, specifically:

In order to protect public repositories for malicious users we run all pull request workflows raised from repository forks with a read-only token and no access to secrets.

[rieschl]

See https://github.blog/2020-08-03-github-actions-improvements-for-fork-and-pull-request-workflows/, specifically:

In order to protect public repositories for malicious users we run all pull request workflows raised from repository forks with a read-only token and no access to secrets.

So, it is read-only. Probably, it could interfere with coveralls?

[Ocramius]

TBH, I wouldn't cry much if coverall disappeared :D

Future scope will likely include @infection, which is a better metric of test quality.

[rieschl]

Yes, infection is cool 🙂
I also think that the full coverage data is overkill for CI. We use that action to just show the coverage in % in a PR comment and fail the workflow if it is below some threshold. But that also doesn't work with a PR from a forked repo 😉

[boesing]

We are facing circular dependency issues in some packages.
We recently ran into issues with this in laminas/laminas-validator#86 and thus, I want to propose some extra handling for composer so we can resolve circular dependencies a bit better.

When a PR is created, GHA can grab the target branch from ${{ github.event.pull_request.base.ref }}. We could pass that value as an environment variable COMPOSER_ROOT_VERSION so composer knows which version is currently on.

This is only necessary for PRs as they usually are not being named as semver versions.

We should keep this in mind to avoid having commits in a PR which provides the target version in a way we might miss when merging PRs.

[weierophinney]

@boesing That can be accomplished relatively easily in GHA:

env:
    BRANCH_NAME: ${{ github.base_ref }}
run: |
    export COMPOSER_ROOT_VERSION=${BRANCH_NAME/%.x/.99}
    # do other commands here

I think this would be something we document, but not include by default; there's only a small number of repos where this is an issue, and I'd rather we have circular references cause errors, and force us to address them, then to put in logic that masks them from us.

[weierophinney]

TBH, I wouldn't cry much if coverall disappeared :D

I'd be okay taking that section out of the templates. I hadn't been aware there would be issues pushing by non-maintainers, so if that's the case, we should remove that section.

[Ocramius]

Discussed in chat: the biggest issue I see right now is having to copy massive YAML files across dozens of repositories, which is feasible, but it's gonna be extremely draining for everyone.

In addition to that, depending which versions of base dependencies are needed, more complexity and small variations can flow in: that's my biggest problem with this.

[Ocramius]

@WyriHaximus has been very creative, and did some work to abstract away things like determining which PHP versions to test: https://github.com/WyriHaximus/php-test-utilities/blob/acf6b25513aba198717ab57d4382e1677ffafcc3/.github/workflows/ci.yml#L20-L21

[WyriHaximus]

@Ocramius happy to help out with this 👍

[weierophinney]

Please see laminas/laminas-eventmanager#15 for a revised approach that builds on the work of @WyriHaximus .

[weierophinney]

Closing, as this has been accepted, and we've begun rollout.