diff --git a/.gitattributes b/.gitattributes index 083e1747cba..5f8117153f5 100644 --- a/.gitattributes +++ b/.gitattributes @@ -16,7 +16,6 @@ tests export-ignore Dockerfile.ci export-ignore ISSUE_TRIAGE_PROCESS.rst export-ignore -PULL_REQUEST_WORKFLOW.rst export-ignore STATIC_CODE_CHECKS.rst export-ignore TESTING.rst export-ignore LOCAL_VIRTUALENV.rst export-ignore diff --git a/.github/actions/checks-action b/.github/actions/checks-action deleted file mode 160000 index 9f02872da71..00000000000 --- a/.github/actions/checks-action +++ /dev/null @@ -1 +0,0 @@ -Subproject commit 9f02872da71b6f558c6a6f190f925dde5e4d8798 diff --git a/.github/actions/label-when-approved-action b/.github/actions/label-when-approved-action deleted file mode 160000 index 0058d0094da..00000000000 --- a/.github/actions/label-when-approved-action +++ /dev/null @@ -1 +0,0 @@ -Subproject commit 0058d0094da27e116fad6e0da516ebe1107f26de diff --git a/.github/workflows/label_when_reviewed.yml b/.github/workflows/label_when_reviewed.yml deleted file mode 100644 index 189a2d7343b..00000000000 --- a/.github/workflows/label_when_reviewed.yml +++ /dev/null @@ -1,28 +0,0 @@ -# Licensed to the Apache Software Foundation (ASF) under one -# or more contributor license agreements. See the NOTICE file -# distributed with this work for additional information -# regarding copyright ownership. The ASF licenses this file -# to you under the Apache License, Version 2.0 (the -# "License"); you may not use this file except in compliance -# with the License. You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, -# software distributed under the License is distributed on an -# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY -# KIND, either express or implied. See the License for the -# specific language governing permissions and limitations -# under the License. -# ---- -name: Label when reviewed -on: pull_request_review # yamllint disable-line rule:truthy -jobs: - - label-when-reviewed: - name: "Label PRs when reviewed" - runs-on: ubuntu-20.04 - steps: - - name: "Do nothing. Only trigger corresponding workflow_run event" - run: echo diff --git a/.github/workflows/label_when_reviewed_workflow_run.yml b/.github/workflows/label_when_reviewed_workflow_run.yml deleted file mode 100644 index b84ab34a79e..00000000000 --- a/.github/workflows/label_when_reviewed_workflow_run.yml +++ /dev/null @@ -1,177 +0,0 @@ -# Licensed to the Apache Software Foundation (ASF) under one -# or more contributor license agreements. See the NOTICE file -# distributed with this work for additional information -# regarding copyright ownership. The ASF licenses this file -# to you under the Apache License, Version 2.0 (the -# "License"); you may not use this file except in compliance -# with the License. You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, -# software distributed under the License is distributed on an -# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY -# KIND, either express or implied. See the License for the -# specific language governing permissions and limitations -# under the License. -# ---- -name: Label when reviewed workflow run -on: # yamllint disable-line rule:truthy - workflow_run: - workflows: ["Label when reviewed"] - types: ['requested'] -permissions: - # All other permissions are set to none - checks: write - contents: read - pull-requests: write -jobs: - - label-when-reviewed: - name: "Label PRs when reviewed workflow run" - runs-on: ubuntu-20.04 - outputs: - labelSet: ${{ steps.label-when-reviewed.outputs.labelSet }} - steps: - - name: "Checkout ${{ github.ref }} ( ${{ github.sha }} )" - uses: actions/checkout@v2 - with: - persist-credentials: false - submodules: recursive - - name: "Get information about the original trigger of the run" - uses: ./.github/actions/get-workflow-origin - id: source-run-info - with: - token: ${{ secrets.GITHUB_TOKEN }} - sourceRunId: ${{ github.event.workflow_run.id }} - - name: Initiate Selective Build check - uses: ./.github/actions/checks-action - id: selective-build-check - with: - token: ${{ secrets.GITHUB_TOKEN }} - name: "Selective build check" - status: "in_progress" - sha: ${{ steps.source-run-info.outputs.sourceHeadSha }} - details_url: https://github.com/${{ github.repository }}/actions/runs/${{ github.run_id }} - output: > - {"summary": - "Checking selective status of the build in - [the run](https://github.com/${{ github.repository }}/actions/runs/${{ github.run_id }}) - "} - - name: > - Event: ${{ steps.source-run-info.outputs.sourceEvent }} - Repo: ${{ steps.source-run-info.outputs.sourceHeadRepo }} - Branch: ${{ steps.source-run-info.outputs.sourceHeadBranch }} - Run id: ${{ github.run_id }} - Source Run id: ${{ github.event.workflow_run.id }} - Sha: ${{ github.sha }} - Source Sha: ${{ steps.source-run-info.outputs.sourceHeadSha }} - Merge commit Sha: ${{ steps.source-run-info.outputs.mergeCommitSha }} - Target commit Sha: ${{ steps.source-run-info.outputs.targetCommitSha }} - run: printenv - - name: > - Fetch incoming commit ${{ steps.source-run-info.outputs.targetCommitSha }} with its parent - uses: actions/checkout@v2 - with: - ref: ${{ steps.source-run-info.outputs.targetCommitSha }} - fetch-depth: 2 - persist-credentials: false - # checkout the main branch again, to use the right script in main workflow - - name: "Checkout ${{ github.ref }} ( ${{ github.sha }} )" - uses: actions/checkout@v2 - with: - persist-credentials: false - submodules: recursive - - name: "Setup python" - uses: actions/setup-python@v2 - with: - # We do not have output from selective checks yet, so we need to hardcode python - python-version: 3.7 - cache: 'pip' - cache-dependency-path: ./dev/breeze/setup* - - run: ./scripts/ci/install_breeze.sh - - name: Selective checks - id: selective-checks - env: - PR_LABELS: "${{ steps.source-run-info.outputs.pullRequestLabels }}" - COMMIT_REF: "${{ steps.source-run-info.outputs.targetCommitSha }}" - run: breeze selective-check - - name: "Label when approved by committers for PRs that require full tests" - uses: ./.github/actions/label-when-approved-action - id: label-full-test-prs-when-approved-by-commiters - if: > - steps.selective-checks.outputs.run-tests == 'true' && - contains(steps.selective-checks.outputs.test-types, 'Core') - with: - token: ${{ secrets.GITHUB_TOKEN }} - label: 'full tests needed' - require_committers_approval: 'true' - remove_label_when_approval_missing: 'false' - pullRequestNumber: ${{ steps.source-run-info.outputs.pullRequestNumber }} - comment: > - The PR most likely needs to run full matrix of tests because it modifies parts of the core - of Airflow. However, committers might decide to merge it quickly and take the risk. - If they don't merge it quickly - please rebase it to the latest main at your convenience, - or amend the last commit of the PR, and push it with --force-with-lease. - - name: "Initiate GitHub Check forcing rerun of SH ${{ github.event.pull_request.head.sha }}" - uses: ./.github/actions/checks-action - id: full-test-check - if: steps.label-full-test-prs-when-approved-by-commiters.outputs.labelSet == 'true' - with: - token: ${{ secrets.GITHUB_TOKEN }} - name: "Please rebase or amend, and force push the PR to run full tests" - status: "in_progress" - sha: ${{ steps.source-run-info.outputs.sourceHeadSha }} - details_url: https://github.com/${{ github.repository }}/actions/runs/${{ github.run_id }} - output: > - {"summary": - "The PR likely needs to run all tests! This was determined via selective check in - [the run](https://github.com/${{ github.repository }}/actions/runs/${{ github.run_id }}) - "} - - name: "Label when approved by committers for PRs that do not require full tests" - uses: ./.github/actions/label-when-approved-action - id: label-simple-test-prs-when-approved-by-commiters - if: > - steps.selective-checks.outputs.run-tests == 'true' && - ! contains(steps.selective-checks.outputs.test-types, 'Core') - with: - token: ${{ secrets.GITHUB_TOKEN }} - label: 'okay to merge' - require_committers_approval: 'true' - pullRequestNumber: ${{ steps.source-run-info.outputs.pullRequestNumber }} - comment: > - The PR is likely OK to be merged with just subset of tests for default Python and Database - versions without running the full matrix of tests, because it does not modify the core of - Airflow. If the committers decide that the full tests matrix is needed, they will add the label - 'full tests needed'. Then you should rebase to the latest main or amend the last commit - of the PR, and push it with --force-with-lease. - - name: "Label when approved by committers for PRs that do not require tests at all" - uses: ./.github/actions/label-when-approved-action - id: label-no-test-prs-when-approved-by-commiters - if: steps.selective-checks.outputs.run-tests != 'true' - with: - token: ${{ secrets.GITHUB_TOKEN }} - label: 'okay to merge' - pullRequestNumber: ${{ steps.source-run-info.outputs.pullRequestNumber }} - require_committers_approval: 'true' - comment: > - The PR is likely ready to be merged. No tests are needed as no important environment files, - nor python files were modified by it. However, committers might decide that full test matrix is - needed and add the 'full tests needed' label. Then you should rebase it to the latest main - or amend the last commit of the PR, and push it with --force-with-lease. - - name: Update Selective Build check - uses: ./.github/actions/checks-action - if: always() - with: - token: ${{ secrets.GITHUB_TOKEN }} - check_id: ${{ steps.selective-build-check.outputs.check_id }} - status: "completed" - sha: ${{ steps.source-run-info.outputs.sourceHeadSha }} - conclusion: ${{ job.status }} - details_url: https://github.com/${{ github.repository }}/actions/runs/${{ github.run_id }} - output: > - {"summary": - "Checking selective status of the build completed in - [the run](https://github.com/${{ github.repository }}/actions/runs/${{ github.run_id }}) - "} diff --git a/.gitmodules b/.gitmodules index e03978e2636..aa1358f8849 100644 --- a/.gitmodules +++ b/.gitmodules @@ -1,9 +1,6 @@ [submodule ".github/actions/get-workflow-origin"] path = .github/actions/get-workflow-origin url = https://github.com/potiuk/get-workflow-origin -[submodule ".github/actions/checks-action"] - path = .github/actions/checks-action - url = https://github.com/LouisBrunner/checks-action [submodule ".github/actions/configure-aws-credentials"] path = .github/actions/configure-aws-credentials url = https://github.com/aws-actions/configure-aws-credentials @@ -13,6 +10,3 @@ [submodule ".github/actions/github-push-action"] path = .github/actions/github-push-action url = https://github.com/ad-m/github-push-action -[submodule ".github/actions/label-when-approved-action"] - path = .github/actions/label-when-approved-action - url = https://github.com/TobKed/label-when-approved-action diff --git a/CI.rst b/CI.rst index 7798b077acf..c0585984492 100644 --- a/CI.rst +++ b/CI.rst @@ -426,12 +426,6 @@ CI, Production Images as well as base Python images that are also cached in the Also for those builds we only execute Python tests if important files changed (so for example if it is "no-code" change, no tests will be executed. -The workflow involved in Pull Requests review and approval is a bit more complex than simple workflows -in most of other projects because we've implemented some optimizations related to efficient use -of queue slots we share with other Apache Software Foundation projects. More details about it -can be found in `PULL_REQUEST_WORKFLOW.rst `_. - - Direct Push/Merge Run --------------------- diff --git a/CONTRIBUTING.rst b/CONTRIBUTING.rst index 4ec19f86ac4..c2e5d53eded 100644 --- a/CONTRIBUTING.rst +++ b/CONTRIBUTING.rst @@ -359,33 +359,6 @@ Step 4: Prepare PR PR guidelines described in `pull request guidelines <#pull-request-guidelines>`_. Create Pull Request! Make yourself ready for the discussion! -5. Depending on "scope" of your changes, your Pull Request might go through one of few paths after approval. - We run some non-standard workflow with high degree of automation that allows us to optimize the usage - of queue slots in GitHub Actions. Our automated workflows determine the "scope" of changes in your PR - and send it through the right path: - - * In case of a "no-code" change, approval will generate a comment that the PR can be merged and no - tests are needed. This is usually when the change modifies some non-documentation related RST - files (such as this file). No python tests are run and no CI images are built for such PR. Usually - it can be approved and merged few minutes after it is submitted (unless there is a big queue of jobs). - - * In case of change involving python code changes or documentation changes, a subset of full test matrix - will be executed. This subset of tests perform relevant tests for single combination of python, backend - version and only builds one CI image and one PROD image. Here the scope of tests depends on the - scope of your changes: - - * when your change does not change "core" of Airflow (Providers, CLI, WWW, Helm Chart) you will get the - comment that PR is likely ok to be merged without running "full matrix" of tests. However decision - for that is left to committer who approves your change. The committer might set a "full tests needed" - label for your PR and ask you to rebase your request or re-run all jobs. PRs with "full tests needed" - run full matrix of tests. - - * when your change changes the "core" of Airflow you will get the comment that PR needs full tests and - the "full tests needed" label is set for your PR. Additional check is set that prevents from - accidental merging of the request until full matrix of tests succeeds for the PR. - - More details about the PR workflow be found in `PULL_REQUEST_WORKFLOW.rst `_. - Step 5: Pass PR Review ---------------------- diff --git a/PULL_REQUEST_WORKFLOW.rst b/PULL_REQUEST_WORKFLOW.rst deleted file mode 100644 index 7d7a7860fdc..00000000000 --- a/PULL_REQUEST_WORKFLOW.rst +++ /dev/null @@ -1,151 +0,0 @@ - .. Licensed to the Apache Software Foundation (ASF) under one - or more contributor license agreements. See the NOTICE file - distributed with this work for additional information - regarding copyright ownership. The ASF licenses this file - to you under the Apache License, Version 2.0 (the - "License"); you may not use this file except in compliance - with the License. You may obtain a copy of the License at - - .. http://www.apache.org/licenses/LICENSE-2.0 - - .. Unless required by applicable law or agreed to in writing, - software distributed under the License is distributed on an - "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - KIND, either express or implied. See the License for the - specific language governing permissions and limitations - under the License. - -.. contents:: :local: - -Why non-standard pull request workflow? ---------------------------------------- - -This document describes the Pull Request Workflow we've implemented in Airflow. The workflow is slightly -more complex than regular workflow you might encounter in most of the projects because after experiencing -some huge delays in processing queues in October 2020 with GitHub Actions, we've decided to optimize the -workflow to minimize the use of GitHub Actions build time by utilising selective approach on which tests -and checks in the CI system are run depending on analysis of which files changed in the incoming PR and -allowing the Committers to control the scope of the tests during the approval/review process. - -Just to give a bit of context, we started off with the approach that we always run all tests for all the -incoming PRs, however due to our matrix of tests growing, this approach did not scale with the increasing -number of PRs and when we had to compete with other Apache Software Foundation projects for the 180 -slots that are available for the whole organization. More Apache Software Foundation projects started -to use GitHub Actions and we've started to experience long queues when our jobs waited for free slots. - -We approached the problem by: - -1) Improving mechanism of cancelling duplicate workflow runs more efficiently in case of queue conditions - (duplicate workflow runs are generated when someone pushes a fixup quickly - leading to running both - out-dated and current run to completion, taking precious slots. This has been implemented by improving - `cancel-workflow-run `_ action we are using. In version - 4.1 it got a new feature of cancelling all duplicates even if there is a long queue of builds. - -2) Heavily decreasing strain on the GitHub Actions jobs by introducing selective checks - mechanism - to control which parts of the tests are run during the tests. This is implemented by the - ``breeze selective-check`` command. It selectively chooses which tests should be run in the PR based on - type of the PR and its content. More about it can be found in - `Selective checks `_ - -3) Even more optimisation came from limiting the scope of tests to only "default" matrix parameters. So far - in Airflow we always run all tests for all matrix combinations. The primary matrix components are: - - * Python versions (currently 3.7, 3.8, 3.9, 3.10) - * Backend types (currently MySQL/Postgres) - * Backed version (currently MySQL 5.7, MySQL 8, Postgres 13 - - We've decided that instead of running all the combinations of parameters for all matrix component we will - only run default values (Python 3.7, Mysql 5.7, Postgres 13) for all PRs which are not approved yet by - the committers. This has a nice effect, that full set of tests (though with limited combinations of - the matrix) are still run in the CI for every Pull Request that needs tests at all - allowing the - contributors to make sure that their PR is "good enough" to be reviewed. - - Even after approval, the automated workflows we've implemented, check if the PR seems to need - "full test matrix" and provide helpful information to both contributors and committers in the form of - explanatory comments and labels set automatically showing the status of the PR. Committers have still - control whether they want to merge such requests automatically or ask for rebase or re-run the tests - and run "full tests" by applying the "full tests needed" label and re-running such request. - The "full tests needed" label is also applied automatically after approval when the change touches - the "core" of Airflow - also a separate check is added to the PR so that the "merge" button status - will indicate to the committer that full tests are still needed. The committer might still decide, - whether to merge such PR without the "full matrix". The "escape hatch" we have - i.e. running the full - matrix of tests in the "merge push" will enable committers to catch and fix such problems quickly. - More about it can be found in `Approval workflow and Matrix tests <#approval-workflow-and-matrix-tests>`_ - chapter. - -4) We've also applied (and received) funds to run self-hosted runners. They are used for ``main`` runs - and whenever the PRs are done by one of the maintainers. Maintainers can force using Public GitHub runners - by applying "use public runners" label to the PR before submitting it. - - -Approval Workflow and Matrix tests ----------------------------------- - -As explained above the approval and matrix tests workflow works according to the algorithm below: - -1) In case of "no-code" changes - so changes that do not change any of the code or environment of - the application, no test are run (this is done via selective checks). Also no CI/PROD images are - build saving extra minutes. Such build takes less than 2 minutes currently and only few jobs are run - which is a very small fraction of the "full build" time. - -2) When new PR is created, only a "default set" of matrix test are running. Only default - values for each of the parameters are used effectively limiting it to running matrix builds for only - one python version and one version of each of the backends. In this case only one CI and one PROD - image is built, saving precious job slots. This build takes around 50% less time than the "full matrix" - build. - -3) When such PR gets approved, the system further analyses the files changed in this PR and further - decision is made that should be communicated to both Committer and Reviewer. - -3a) In case of "no-code" builds, a message is communicated that the PR is ready to be merged and - no tests are needed. - -.. image:: images/pr/pr-no-tests-needed-comment.png - :align: center - :alt: No tests needed for "no-code" builds - -3b) In case of "non-core" builds a message is communicated that such PR is likely OK to be merged as is with - limited set of tests, but that the committer might decide to re-run the PR after applying - "full tests needed" label, which will trigger full matrix build for tests for this PR. The committer - might make further decision on what to do with this PR. - -.. image:: images/pr/pr-likely-ok-to-merge.png - :align: center - :alt: Likely ok to merge the PR with only small set of tests - -3c) In case of "core" builds (i. e. when the PR touches some "core" part of Airflow) a message is - communicated that this PR needs "full test matrix", the "full tests needed" label is applied - automatically and either the contributor might rebase the request to trigger full test build or the - committer might re-run the build manually to trigger such full test rebuild. Also a check "in-progress" - is added, so that the committer realises that the PR is not yet "green to merge". Pull requests with - "full tests needed" label always trigger the full matrix build when rebased or re-run so if the - PR gets rebased, it will continue triggering full matrix build. - -.. image:: images/pr/pr-full-tests-needed.png - :align: center - :alt: Full tests are needed for the PR - -4) If this or another committer "request changes" in a previously approved PR with "full tests needed" - label, the bot automatically removes the label, moving it back to "run only default set of parameters" - mode. For PRs touching core of airflow once the PR gets approved back, the label will be restored. - If it was manually set by the committer, it has to be restored manually. - -.. note:: Note that setting the labels and adding comments might be delayed, due to limitation of GitHub Actions, - in case of queues, processing of Pull Request reviews might take some time, so it is advised not to merge - PR immediately after approval. Luckily, the comments describing the status of the PR trigger notifications - for the PRs and they provide good "notification" for the committer to act on a PR that was recently - approved. - -The PR approval workflow is possible thanks to two custom GitHub Actions we've developed: - -* `Get workflow origin `_ -* `Label when approved `_ - - -Next steps ----------- - -We are planning to also propose the approach to other projects from Apache Software Foundation to -make it a common approach, so that our effort is not limited only to one project. - -Discussion about it in `this discussion `_ diff --git a/TESTING.rst b/TESTING.rst index 2271e73ecfd..4f7de58b767 100644 --- a/TESTING.rst +++ b/TESTING.rst @@ -487,8 +487,6 @@ This is done for three reasons: 1. in order to selectively run only subset of the test types for some PRs 2. in order to allow parallel execution of the tests on Self-Hosted runners -For case 1. see `Pull Request Workflow `_ for details. - For case 2. We can utilise memory and CPUs available on both CI and local development machines to run test in parallel. This way we can decrease the time of running all tests in self-hosted runners from 60 minutes to ~15 minutes. diff --git a/dev/airflow-github b/dev/airflow-github index 5dc5f5bd9d4..fcda3355d91 100755 --- a/dev/airflow-github +++ b/dev/airflow-github @@ -126,7 +126,6 @@ def is_core_commit(files: List[str]) -> bool: "CONTRIBUTORS_QUICK_START.rst", "IMAGES.rst", "LOCAL_VIRTUALENV.rst", - "PULL_REQUEST_WORKFLOW.rst", "INTHEWILD.md", "INSTALL", "README.md", diff --git a/images/pr/pr-full-tests-needed.png b/images/pr/pr-full-tests-needed.png deleted file mode 100644 index c863153d069..00000000000 Binary files a/images/pr/pr-full-tests-needed.png and /dev/null differ diff --git a/images/pr/pr-likely-ok-to-merge.png b/images/pr/pr-likely-ok-to-merge.png deleted file mode 100644 index 9c04dee4221..00000000000 Binary files a/images/pr/pr-likely-ok-to-merge.png and /dev/null differ diff --git a/images/pr/pr-no-tests-needed-comment.png b/images/pr/pr-no-tests-needed-comment.png deleted file mode 100644 index 78a11818687..00000000000 Binary files a/images/pr/pr-no-tests-needed-comment.png and /dev/null differ