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

Skips provider package builds and provider tests for non-master #14996

Conversation

potiuk
Copy link
Member

@potiuk potiuk commented Mar 25, 2021

This PR skips building Provider packages for branches different
than master. Provider packages are always released from master
but never from any other branch so there is no point in running
the package building and tests there


^ 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.

@potiuk potiuk requested review from ashb and kaxil as code owners March 25, 2021 00:50
@potiuk potiuk requested review from XD-DENG and houqp March 25, 2021 00:50
@@ -504,12 +505,13 @@ ${{ hashFiles('.pre-commit-config.yaml') }}"
strategy:
matrix:
package-format: ['wheel', 'sdist']
if: needs.build-info.outputs.image-build == 'true'
if: needs.build-info.outputs.image-build == 'true' && needs.build-info.outputs.default-branch == 'master'
Copy link
Member

Choose a reason for hiding this comment

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

Why not just the following:

Suggested change
if: needs.build-info.outputs.image-build == 'true' && needs.build-info.outputs.default-branch == 'master'
if: needs.build-info.outputs.image-build == 'true' && github.ref == 'refs/heads/master'

Copy link
Member Author

@potiuk potiuk Mar 25, 2021

Choose a reason for hiding this comment

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

OK. You asked this question, be prepared for the answer :). It's long, sorry, but GitHub did NOT make it easy.

Beware. It's long even if the question is simple.

The problem is that if you are running direct push (i.e. merge to master) you have the actual master branch in github.ref, indeed, however it looks completely differently in other workflows.

When you run pull_request workflow, github ref is something like refs/pull/14996/merge. In this case you should look for github.base_ref instead:

github.base_ref	string	The base_ref or target branch of the pull request in a workflow run. This property is
 only available when the event that triggers a workflow run is a pull_request.

And - of course - it is not documented.

And ... wait for it .. base_ref is NOT set in direct pushes at all (i.e. when you merge) so if you would like to use this information, your condition will start looking like:

if github.event != 'pull_request' && github.base_ref == 'master' || github.ref == 'master'  

But, unfortunately, this is just a beginning. I am not 100% sure if we are covered here because github.ref behaviour forpull_request workflows is not documented (many of the values in GitHub context are poorly documented or not documented at all and I had to reverse-engineer them).

What's more - in workflow_run workflow you have no information whatsoever what was the base branch (and for a change workflow_run for every push and every PR runs always with master github.ref, no matter what is the target branch. Not even when you query the "source_run" directly via GitHub API you can find the base branch. This is because (surprise-surprise) when you query the "triggering" workflow from the "triggered" one - you have no access to the "context" of the triggering run (obviously). You only can see whatever the API returns you and the last time when I checked there was no information about the "base_branch" there. It could have changed since though.

So what you have to do (I did not find a better way) is that you have to find the original PR via GitHub API and retrieve information from there. But in order to retrieve the information from there you need to iterate through open PRs and match the PR by branch + repo (I am already doing it to be able to push "Status checks" to the PR. Yes. I know, it is insane. But I could not find another way.

If you are adventurous and want to look yourself - in every job we have this step starting from "Event " ... where I dump the context - this makes reverse-engineering much easier (but none more pleasant).

AFAIK there is no sane way to get the 'base" branch.

That's why we have DEFAULT_BRANCH in _initialization.sh that we keep different in every branch and I am using it. And exposing it as "output" seems like much simpler thing to do.

I would be super happy if you could find an easier way here.

Copy link
Member

Choose a reason for hiding this comment

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

At this point, I just hope GH Api become more mature and provide detailed info for use in workflows, any who as we don't know when it would happen let's merge this one in

@github-actions
Copy link

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

@github-actions
Copy link

The Workflow run is cancelling this PR. Building image for the PR has been cancelled

@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 Mar 25, 2021
This PR skips building Provider packages for branches different
than master. Provider packages are always released from master
but never from any other branch so there is no point in running
the package building and tests there
@potiuk potiuk force-pushed the run-provider-packages-builds-and-tests-only-for-master-branch branch from df368f1 to d3bc0a1 Compare March 25, 2021 16:14
@potiuk potiuk merged commit db9febd into apache:master Mar 25, 2021
@potiuk potiuk deleted the run-provider-packages-builds-and-tests-only-for-master-branch branch March 25, 2021 19:31
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