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

Streamline Build Images workflow using new GitHub Actions features #15944

Merged
merged 3 commits into from
May 25, 2021

Conversation

ashb
Copy link
Member

@ashb ashb commented May 19, 2021

Use pull_request_target event for building images, and concurrency to
automatically cancel old jobs for PRs.

This means that:

  • GitHub will automatically cancel old jobs for us, so we don't have to
    handle that ourselves (removes most of the use of the
    cancel-workflow-action)

    New pushes show this for a little while

    2021-05-19_15-26

    And then the old job shows this as the reason

    image

  • GitHub displays these checks directly on the PR, but it is still run
    in the context of our repo, meaning it has access write to our
    repo/access to secrets etc.

    2021-05-19_15-45

  • Since it shows up directly on the PR checks, we don't need to create the
    check in the "CI" workflow to show the status of the Image Build.

  • We also don't need to post the comment saying why it failed, as the
    Build Image status will show up directly there

  • Since pull_request_target has information about the PR in the
    github.event context, we don't need the complex mechanism to find
    the "other" PR, we can do a fairly simple API request and filter by
    the commit SHA to find and cancel to CI workflow job. (This removes
    the final use of the cancel-workflow-action)

One change I had to make here what tag we use for Docker images we build
and push up. Previously we used the "source run ID" (i.e. the id of the
CI run) but with pull_request_target we don't have that anymore. We
could use the same API mechanism we do to cancel to find the target job,
but the only requirement here is for an ID that both jobs know -- the
SHA of the PR branch fills that need

Extra side benefits of this:

  • The sidebar of commits in main branch aren't "polluted" with Build
    Images for PRs like they were previously.

Still do do:

  • Update breeze to pull the new image tag format when given a numeric id (use API to look up sha that a build is for) Update docs to remove references to RUN_ID
  • Delete old build-image-workflow-run workflow file

Use `pull_request_target` event for building images, and `concurrency` to
automatically cancel old jobs for PRs.

This means that:

- GitHub will automatically cancel old jobs for us, so we don't have to
  handle that ourselves (removes most of the use of the
  cancel-workflow-action)

- GitHub displays these checks directly on the PR, but it is still run
  in the context of our repo, meaning it has access write to our
  repo/access to secrets etc.

- Since it shows up directly on the PR checks, we don't need to create the
  check in the "CI" workflow to show the status of the Image Build.

- We also don't need to post the comment saying _why_ it failed, as the
  Build Image status will show up directly there

- Since `pull_request_target` has information about the PR in the
  `github.event` context, we don't need the complex mechanism to find
  the "other" PR, we can do a fairly simple API request and filter by
  the commit SHA to find and cancel to CI workflow job. (This removes
  the final use of the cancel-workflow-action)

One change I had to make here what tag we use for Docker images we build
and push up. Previously we used the "source run ID" (i.e. the id of the
CI run) but with pull_request_target we don't have that anymore. We
could use the same API mechanism we do to cancel to find the target job,
but the only requirement here is for an ID that both jobs know -- the
SHA of the PR branch fills that need

Extra side benefits of this:

- The sidebar of commits in main branch aren't "polluted" with Build
  Images for PRs like they were previously.
@ashb
Copy link
Member Author

ashb commented May 19, 2021

I've been testing this out on my fork here ashb#9 -- that is a PR opened from a different user.

Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

COOL. ❤️ It. The concurrency removes major pain out with the cancelling and having it all in one PR makes it cool.

I love to retire the total madness of the complexity of cancelling because of lack of support from GitHub Actions so far :)

Good stuff @ashb !

@potiuk
Copy link
Member

potiuk commented May 19, 2021

Update breeze to pull the new image tag format when given a numeric id (use API to look up sha that a build is for)

Ths might be not needed. We just need to print SHA in output of tests in instructions and we can use SHA directly. No need to use build ID, maybe just rename the parameter

@ashb ashb marked this pull request as ready for review May 20, 2021 11:43
@ashb ashb requested a review from kaxil as a code owner May 20, 2021 11:43
notifyPRCancel: true
jobNameRegexps: >
["^Pylint$", "^Static checks", "^Build docs$", "^Spell check docs$",
"^Provider packages", "^Checks: Helm tests$", "^Test OpenAPI*"]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, this step is still probably needed, meaning we can't remove the cancel-workflow_runs action entirely.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's an optimisation only, and I am not sure if thats' so much needed after we sped up most of the builds.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Selective checks already take care about most savings we have.

@potiuk
Copy link
Member

potiuk commented May 25, 2021

Happy to merge this one even without the "fast-fail" for the names of the jobs. I have already put the "deprecation notice" in my action : https://github.com/potiuk/cancel-workflow-runs#readme after the concurrency feature added by GitHub Actions..

One less thing to maintain :)

@ashb
Copy link
Member Author

ashb commented May 25, 2021

@potiuk Cool, can you give this a ✅ then please?

And then once merged lets see if we need every open PR to rebase to pick it up or not :/

@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 master at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

@github-actions github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label May 25, 2021
@potiuk
Copy link
Member

potiuk commented May 25, 2021

I think no rebase is neeed. It should just work . The name of images did not change, and this is the only interface between the workflows.

@ashb
Copy link
Member Author

ashb commented May 25, 2021

🤞🏻

@ashb ashb merged commit 9c98a60 into apache:master May 25, 2021
@ashb ashb deleted the pull-request-target-build-images branch May 25, 2021 14:38
kaxil added a commit to astronomer/airflow that referenced this pull request May 31, 2021
I think this was added as a DEBUG step which was forgotten to remove
in apache#15944
kaxil added a commit that referenced this pull request May 31, 2021
I think this was added as a DEBUG step which was forgotten to remove
in #15944
potiuk pushed a commit to potiuk/airflow that referenced this pull request Jun 22, 2021
…pache#15944)

Use `pull_request_target` event for building images, and `concurrency` to
automatically cancel old jobs for PRs.

This means that:

- GitHub will automatically cancel old jobs for us, so we don't have to
  handle that ourselves (removes most of the use of the
  cancel-workflow-action)

- GitHub displays these checks directly on the PR, but it is still run
  in the context of our repo, meaning it has access write to our
  repo/access to secrets etc.

- Since it shows up directly on the PR checks, we don't need to create the
  check in the "CI" workflow to show the status of the Image Build.

- We also don't need to post the comment saying _why_ it failed, as the
  Build Image status will show up directly there

- Since `pull_request_target` has information about the PR in the
  `github.event` context, we don't need the complex mechanism to find
  the "other" PR, we can do a fairly simple API request and filter by
  the commit SHA to find and cancel to CI workflow job. (This removes
  the final use of the cancel-workflow-action)

One change I had to make here what tag we use for Docker images we build
and push up. Previously we used the "source run ID" (i.e. the id of the
CI run) but with pull_request_target we don't have that anymore. We
could use the same API mechanism we do to cancel to find the target job,
but the only requirement here is for an ID that both jobs know -- the
SHA of the PR branch fills that need

Extra side benefits of this:

- The sidebar of commits in main branch aren't "polluted" with Build
  Images for PRs like they were previously.

(cherry picked from commit 9c98a60)
potiuk pushed a commit to potiuk/airflow that referenced this pull request Jun 22, 2021
I think this was added as a DEBUG step which was forgotten to remove
in apache#15944

(cherry picked from commit 19a9fc5)
leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Sep 17, 2021
I think this was added as a DEBUG step which was forgotten to remove
in apache/airflow#15944

(cherry picked from commit 19a9fc5273337ca2bb68eb669055487f81fb21d6)

GitOrigin-RevId: 53805f4c027c090b02391e071ea8640e64ef6901
leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Sep 23, 2021
I think this was added as a DEBUG step which was forgotten to remove
in apache/airflow#15944

(cherry picked from commit 19a9fc5273337ca2bb68eb669055487f81fb21d6)

GitOrigin-RevId: 53805f4c027c090b02391e071ea8640e64ef6901
leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Nov 27, 2021
I think this was added as a DEBUG step which was forgotten to remove
in apache/airflow#15944

(cherry picked from commit 19a9fc5273337ca2bb68eb669055487f81fb21d6)

GitOrigin-RevId: 53805f4c027c090b02391e071ea8640e64ef6901
potiuk added a commit to potiuk/airflow that referenced this pull request Jan 18, 2022
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 added a commit that referenced this pull request Jan 19, 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.
leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Mar 10, 2022
I think this was added as a DEBUG step which was forgotten to remove
in apache/airflow#15944

GitOrigin-RevId: 19a9fc5273337ca2bb68eb669055487f81fb21d6
leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Jun 4, 2022
I think this was added as a DEBUG step which was forgotten to remove
in apache/airflow#15944

GitOrigin-RevId: 19a9fc5273337ca2bb68eb669055487f81fb21d6
kosteev pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Jul 10, 2022
I think this was added as a DEBUG step which was forgotten to remove
in apache/airflow#15944

GitOrigin-RevId: 19a9fc5273337ca2bb68eb669055487f81fb21d6
leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Aug 27, 2022
I think this was added as a DEBUG step which was forgotten to remove
in apache/airflow#15944

GitOrigin-RevId: 19a9fc5273337ca2bb68eb669055487f81fb21d6
leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Oct 4, 2022
I think this was added as a DEBUG step which was forgotten to remove
in apache/airflow#15944

GitOrigin-RevId: 19a9fc5273337ca2bb68eb669055487f81fb21d6
aglipska pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Oct 7, 2022
I think this was added as a DEBUG step which was forgotten to remove
in apache/airflow#15944

GitOrigin-RevId: 19a9fc5273337ca2bb68eb669055487f81fb21d6
kaxil added a commit to astronomer/astro-sdk that referenced this pull request Oct 7, 2022
Currently, we have to manually cancel duplicate jobs when we push more commits to existing PRs.

We use GitHub action's "[concurrency](https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#concurrency)" feature in Airflow too for this: apache/airflow#15944

closes #1021
kaxil added a commit to astronomer/astro-sdk that referenced this pull request Oct 7, 2022
Currently, we have to manually cancel duplicate jobs when we push more
commits to existing PRs.

We use GitHub action's
"[concurrency](https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#concurrency)"
feature in Airflow too for this:
apache/airflow#15944

closes #1021
leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Dec 7, 2022
I think this was added as a DEBUG step which was forgotten to remove
in apache/airflow#15944

GitOrigin-RevId: 19a9fc5273337ca2bb68eb669055487f81fb21d6
leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Jan 27, 2023
I think this was added as a DEBUG step which was forgotten to remove
in apache/airflow#15944

GitOrigin-RevId: 19a9fc5273337ca2bb68eb669055487f81fb21d6
kosteev pushed a commit to kosteev/composer-airflow-test-copybara that referenced this pull request Sep 12, 2024
I think this was added as a DEBUG step which was forgotten to remove
in apache/airflow#15944

GitOrigin-RevId: 19a9fc5273337ca2bb68eb669055487f81fb21d6
kosteev pushed a commit to kosteev/composer-airflow-test-copybara that referenced this pull request Sep 13, 2024
I think this was added as a DEBUG step which was forgotten to remove
in apache/airflow#15944

GitOrigin-RevId: 19a9fc5273337ca2bb68eb669055487f81fb21d6
kosteev pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Sep 17, 2024
I think this was added as a DEBUG step which was forgotten to remove
in apache/airflow#15944

GitOrigin-RevId: 19a9fc5273337ca2bb68eb669055487f81fb21d6
kosteev pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Nov 7, 2024
I think this was added as a DEBUG step which was forgotten to remove
in apache/airflow#15944

GitOrigin-RevId: 19a9fc5273337ca2bb68eb669055487f81fb21d6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:dev-tools 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.

2 participants