tempestphp / tempest-framework

The PHP framework that gets out of your way 🌊
https://tempest.stitcher.io
MIT License
634 stars 44 forks source link

[CI] Improvements #317

Closed Treggats closed 1 week ago

Treggats commented 1 month ago

Since we now support multiple databases, our CI should reflect that.

feature let github actions run with the following databases

todo


Proposal to support multiple database dialects

Treggats commented 1 month ago

@aidan-casey is there a reason that 99% of the Quality Github Action workflows are the same?

By applying the following patch they could be merged into one.

patch ```diff diff --git a/.github/workflows/quality-assurance.yml b/.github/workflows/quality-assurance.yml index 34a0520..0cf2004 100644 --- a/.github/workflows/quality-assurance.yml +++ b/.github/workflows/quality-assurance.yml @@ -2,7 +2,11 @@ name: Quality Assurance on: pull_request: - branches: [main] + branches: + - main + push: + branches: + - main workflow_dispatch: jobs: @@ -23,8 +27,9 @@ jobs: - name: Validate Composer run: composer validate - style: + check_style: name: Check Styling + if: ${{ github.event_name == 'pull_request' }} runs-on: ubuntu-latest steps: - name: Checkout code @@ -35,6 +40,24 @@ jobs: with: args: --config=.php-cs-fixer.dist.php --allow-risky=yes --dry-run -v + fix_style: + name: Check Styling + if: ${{ github.event_name == 'push' }} + runs-on: ubuntu-latest + steps: + - name: Checkout code + uses: actions/checkout@v4 + + - name: Run PHP-CS-Fixer + uses: docker://oskarstark/php-cs-fixer-ga + with: + args: --config=.php-cs-fixer.dist.php --allow-risky=yes -v + + - name: Commit Changes + uses: stefanzweifel/git-auto-commit-action@v5 + with: + commit_message: Fixes styling. + phpstan: name: Perform Static Analysis runs-on: ubuntu-latest @@ -96,4 +119,4 @@ jobs: env: COVERALLS_REPO_TOKEN: ${{ secrets.GITHUB_TOKEN }} run: | - php-coveralls --coverage_clover=build/reports/clover.xml --json_path=build/reports/coveralls-upload.json -v \ No newline at end of file + php-coveralls --coverage_clover=build/reports/clover.xml --json_path=build/reports/coveralls-upload.json -v ```
Treggats commented 1 month ago

@brendt how do you feel about adding rector as a job to keep it up to date?

aidan-casey commented 1 month ago

@Treggats they originally started as a single job, but as we considered things we might be looking to add in the future (automatic comments to PRs, etc.) it felt cleaner to split off.

aidan-casey commented 1 month ago

@Treggats does Rector have a CI mode like CS fixer? One of the decisions we made there was that those CI tools would not automatically write back to the repo on PRs, but would fail so people can run and commit them locally. We'd need to be able to maintain that approach, I think.

Treggats commented 1 month ago

@aidan-casey

@Treggats does Rector have a CI mode like CS fixer?

I'm guessing you mean running but not changing anything. It does. (--dry-run)

# the X at the end indicates an error exit code
#
~/P❯❯❯ vendor/bin/rector process --no-ansi --dry-run --no-diffs --no-progress-bar                    ✘ 1

 [OK] 1 file would have been changed (dry-run) by Rector 

About writing back you'd need a separate action for that. Like the fixer has.

but would fail so people can run and commit them locally.

I have the same approach on my work application, though we use Laravel Pint there, which will fail if it find anything

Treggats commented 1 month ago

@aidan-casey

@Treggats they originally started as a single job, but as we considered things we might be looking to add in the future (automatic comments to PRs, etc.) it felt cleaner to split off.

On the subject of splitting of, if we need to have recurring action/jobs/etc. it's possible to create our own custum action and reference that.

Small example

  - name: Deploy to staging
    uses: ./.github/actions/vapor-deploy
    with:
      composer_auth: ${{ secrets.COMPOSER_AUTH }
aidan-casey commented 1 month ago

On the subject of splitting of, if we need to have recurring action/jobs/etc. it's possible to create our own custum action and reference that.

Sure, but now you're still talking a minimum of two (if not three) separate actions. If we end up with three workflows that need the same functionality, it may make sense to refactor that, but I'm not seeing the need today.

Treggats commented 1 month ago

, it may make sense to refactor that, but I'm not seeing the need today.

Same, I just mentioned it as a possibility for in the future :)

brendt commented 1 week ago

I've made it so that MySQL tests work. We still need to update the github action, but I don't know how.

I'm gonna skip on adding pgsql support for now, see #354

Treggats commented 1 week ago

I'll update the GitHub action when I get back

Treggats commented 1 week ago

@brendt I've updated #322 to include MySQL in the CI (also Postgres, but could remove that again if needed) All tests passed

Treggats commented 1 week ago

With the PR being merged, this issue can be closed. The issue with Postgress is going to be addressed with #364