combine ci into 1 job, remove disable flags, seperate out PR and push#178
Draft
jay-oswald wants to merge 2 commits into
Draft
combine ci into 1 job, remove disable flags, seperate out PR and push#178jay-oswald wants to merge 2 commits into
jay-oswald wants to merge 2 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors the reusable GitHub Actions CI workflow to separate pull-request testing from push-time releasing, reduce job overhead by consolidating multiple CI checks into a single job with multiple steps, and simplify/modernize inputs (including deprecating disable_behat in favor of enable_behat).
Changes:
- Split the reusable workflow into a PR workflow (
pr.yml) and a push/release workflow (push.yml), withci.ymlacting as a router. - Consolidate many previously separate CI jobs into a single
plugin_cijob with multiple steps (while keeping matrix jobs for phpunit/behat). - Update input handling and documentation (new/updated inputs; deprecate
disable_behat; update parse-version logic).
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Updates consumer-facing documentation and inputs (including enable_behat + deprecations). |
| .github/workflows/push.yml | Adds a dedicated reusable workflow for push-time eligibility checks and release execution. |
| .github/workflows/pr.yml | Adds a dedicated reusable workflow for PR-time checks, including consolidated plugin_ci steps plus phpunit/behat matrices. |
| .github/workflows/ci.yml | Converts ci.yml into a router that dispatches to PR vs push workflows. |
| .github/actions/parse-version/script.php | Updates env flag parsing and behat enable/disable/deprecation behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+252
to
+254
| if: needs.pre_job.outputs.should_skip != 'true' | ||
| runs-on: ubuntu-latest | ||
| needs: prepare_matrix |
Comment on lines
+398
to
+399
| needs: prepare_matrix | ||
| if: needs.pre_job.outputs.should_skip != 'true' && fromJson(needs.prepare_matrix.outputs.phpunit_matrix).include[0] != null |
Comment on lines
+512
to
+513
| needs: prepare_matrix | ||
| if: needs.pre_job.outputs.should_skip != 'true' && fromJson(needs.prepare_matrix.outputs.behat_matrix).include[0] != null |
| ) | ||
| ) }} | ||
|
|
||
| echo "::set-output name=publishable::$publishable" |
| ) | ||
| ) }} | ||
|
|
||
| echo "::set-output name=publishable::$publishable" |
| env: | ||
| SECRET_TO_CHECK: '${{ secrets.moodle_org_token }}' | ||
| if: ${{ env.SECRET_TO_CHECK != '' }} | ||
| run: echo "::set-output name=has-secrets::true" |
Comment on lines
63
to
67
| ```yaml | ||
| with: | ||
| disable_behat: true | ||
| disable_grunt: true | ||
| enable_behat: false | ||
| moodle_branches: MOODLE_404_STABLE | ||
| extra_plugin_runners: 'moodle-plugin-ci add-plugin catalyst/moodle-local_aws' |
| moodle_branches: ${{ inputs.moodle_branches }} | ||
| min_php: ${{ inputs.min_php }} | ||
| ignore_paths: ${{ inputs.ignore_paths }} | ||
| concurrent_skipping: ${{ inputs.concurrent_skipping }} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR will change quite a few things.
This will clean things up, tests run on PR's and releases run on push.
This means we can merge a PR with failing tests and it will release anyway. Currently it only releases when tests pass, and we sometimes disable tests to release anyway. Which isn't great. Now when code merges it releases, leaving tests up to the PR only.
Each job has ~35s overhead for a 1s job. Slightly less visibility on the PR itself, but much faster overall, with dedicated results still.
We want things passing and in general they shouldn't be disabled. Combined with the first change, if there is an issue with a test we can merge and release anyway.
I have also deprecated
disable_behatin favour ofenable_behatflag.ci.ymlis now mainly a router, this is to avoid needing to change the CI file on every plugin, and still give us push and PR events seperatly.Still needs proper testing as a PR on this branch before merging, but flagging some peope for review as a conceptual stage atm