Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix cancelling of Pull Request builds when image build fails #20939

Merged
merged 1 commit into from
Jan 19, 2022

Conversation

potiuk
Copy link
Member

@potiuk potiuk commented Jan 18, 2022

When image build fails, the pull request that triggered it should
be cancelled. The #15944 introduced rewrite of the GitHub actions
code but by mistake it also introduced a failure in cancelling
the PR workflow by missing pipeline to jq.

In most cases it did not matter, but it cause "wait for images"
in PRs to run far longer than they should be.

This PR restores cancelling feature.


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.

When image build fails, the pull request that triggered it should
be cancelled. The apache#15944 introduced rewrite of the GitHub actions
code but by mistake it also introduced a failure in cancelling
the PR workflow by missing pipeline to jq.

In most cases it did not matter, but it cause "wait for images"
in PRs to run far longer than they should be.

This PR restores cancelling feature.
@potiuk potiuk requested review from ashb and kaxil as code owners January 18, 2022 22:33
@potiuk
Copy link
Member Author

potiuk commented Jan 18, 2022

Hey @ashb - I will test the cancelling after the change but I believe it might explain some "far too long" runnig "wait for image" jobs in case we had breaking "build image" changes in main.

@potiuk potiuk closed this Jan 18, 2022
@potiuk potiuk reopened this Jan 18, 2022
@malthe
Copy link
Contributor

malthe commented Jan 19, 2022

I propose a change to use --jq which is used elsewhere in the script rather than piping.

@potiuk
Copy link
Member Author

potiuk commented Jan 19, 2022

I propose a change to use --jq which is used elsewhere in the script rather than piping.

Thanks @malthe - we are in the process of rewriting all the scripts to python (including those small snippets) as part of #12282 so let's have minimal changes here.

I tested tha in https://github.com/potiuk/airflow/runs/4865079794?check_suite_focus=true and piping works as expected:

  if [[ "push" == 'pull_request_target' ]]; then
    event_filter="event=pull_request&"
  else
    branch="${GITHUB_REF#refs/heads/}"
    event_filter=""
  fi
  
  for cancel_url in $(
      gh api "/repos/$GITHUB_REPOSITORY/actions/runs?${event_filter}branch=${branch}" | \
          jq -r '
            .workflow_runs[] |
            select(.head_sha == $ENV.GITHUB_REGISTRY_PUSH_IMAGE_TAG and .status != "completed") |
            .cancel_url
          ' \
  ); do
    # One of these URls will be _this_ workflow, so lets exclude that!
    [[ $cancel_url  == */$thisRun/* ]] && continue
  
    echo "Cancelling $cancel_url"
    gh api -X POST --silent "$cancel_url"
  done
  shell: /usr/bin/bash -e {0}
  env:
    MOUNT_SELECTED_LOCAL_SOURCES: false
    FORCE_ANSWER_TO_QUESTIONS: yes
    CHECK_IMAGE_FOR_REBUILD: true
    SKIP_CHECK_REMOTE_IMAGE: true
    DB_RESET: true
    VERBOSE: true
    GITHUB_REPOSITORY: potiuk/airflow
    GITHUB_USERNAME: potiuk
    CONSTRAINTS_GITHUB_REPOSITORY: apache/airflow
    GITHUB_TOKEN: ***
    GITHUB_REGISTRY_PULL_IMAGE_TAG: latest
    GITHUB_REGISTRY_WAIT_FOR_IMAGE: false
    INSTALL_PROVIDERS_FROM_SOURCES: true
    AIRFLOW_LOGIN_TO_GITHUB_REGISTRY: true
    GITHUB_REGISTRY_PUSH_IMAGE_TAG: 2050e628f8e5fd18b8b13b5c52dcbf3daad003bc
    branch: 
    thisRun: 1717050085
Cancelling https://api.github.com/repos/potiuk/airflow/actions/runs/1717050086/cancel

@github-actions github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Jan 19, 2022
@github-actions
Copy link

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.

@potiuk potiuk merged commit b171e03 into apache:main Jan 19, 2022
@potiuk potiuk deleted the fix-cancelling-of-failed-image-building branch January 19, 2022 10:13
potiuk added a commit to potiuk/airflow that referenced this pull request Jan 19, 2022
potiuk added a commit that referenced this pull request Jan 19, 2022
@potiuk potiuk restored the fix-cancelling-of-failed-image-building branch April 26, 2022 20:53
@potiuk potiuk deleted the fix-cancelling-of-failed-image-building branch July 29, 2022 20:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
full tests needed We need to run full set of tests for this PR to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants