Skip to content

For v1-10-test PRs and pushes, use target branch scripts for images #12339

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.

Sign up for GitHub

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

Jump to bottom
Merged
potiuk merged 1 commit into from
Nov 13, 2020
Merged

For v1-10-test PRs and pushes, use target branch scripts for images #12339

potiuk merged 1 commit into from
Nov 13, 2020

Conversation

@potiuk
Copy link
Member

@potiuk potiuk commented Nov 13, 2020

Previously, always master scripts were used to build images
for workflow_run, because workflow_run always runs from master
branch. However that causes some surprising effects becuase the
sripts from master had to support both master and 1.10.

This change utilises a new feature in the "get-workflow-origin"
action - to get the target branch of PR and uses ci scripts from that
target branch.

This is perfectly secure, because both v1-10-test, v1-10-stable
and future 2-0 branches can only be updated by committers,
either by direct push or by merge.

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

@github-actions
Copy link

github-actions bot commented Nov 13, 2020

The PR needs to run all tests because it modifies core of Airflow! Please rebase it to latest master or ask committer to re-run it!

@github-actions github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Nov 13, 2020
@potiuk
Copy link
Member Author

potiuk commented Nov 13, 2020 •
edited
Loading

Testing it here:

Direct push to master: https://github.com/potiuk/airflow/actions/runs/361589802

Setting output: sourceHeadRepo: potiuk/airflow
Setting output: sourceHeadBranch: master
Setting output: sourceHeadSha: 040222baa32639b74451dce9c2917bd3483ddb46
Setting output: sourceEvent: push
Setting output: pullRequestNumber: 
Setting output: pullRequestLabels: []
Setting output: mergeCommitSha: 
Setting output: targetCommitSha: 040222baa32639b74451dce9c2917bd3483ddb46
Setting output: targetBranch: master

PR: from a fork to master: https://github.com/potiuk/airflow/actions/runs/361596665 higrys -> potiuk

Setting output: sourceHeadRepo: higrys/airflow
Setting output: sourceHeadBranch: test-8
Setting output: sourceHeadSha: 93a92091e64abb1068608816a4fb5b3e7474149c
Setting output: sourceEvent: pull_request
Setting output: pullRequestNumber: 150
Setting output: pullRequestLabels: []
Setting output: mergeCommitSha: 318fbafce42cb2d9b44b07ec495b8f545a4a87bc
Setting output: targetCommitSha: 318fbafce42cb2d9b44b07ec495b8f545a4a87bc
Setting output: targetBranch: master

Direct push to v1-10-test (stable will work the same): https://github.com/potiuk/airflow/actions/runs/361597050

Setting output: sourceHeadRepo: potiuk/airflow
Setting output: sourceHeadBranch: v1-10-test
Setting output: sourceHeadSha: 040222baa32639b74451dce9c2917bd3483ddb46
Setting output: sourceEvent: push
Setting output: pullRequestNumber: 
Setting output: pullRequestLabels: []
Setting output: mergeCommitSha: 
Setting output: targetCommitSha: 040222baa32639b74451dce9c2917bd3483ddb46
Setting output: targetBranch: v1-10-test

PR from a fork to v1-10-test (stable will work the same): https://github.com/potiuk/airflow/actions/runs/361597777 higrys -> potiuk

Setting output: sourceHeadRepo: higrys/airflow
Setting output: sourceHeadBranch: test-8-v1
Setting output: sourceHeadSha: 93a92091e64abb1068608816a4fb5b3e7474149c
Setting output: sourceEvent: pull_request
Setting output: pullRequestNumber: 151
Setting output: pullRequestLabels: []
Setting output: mergeCommitSha: b2242877cacb038a31c8c6a5d9d32f6df0ecaed9
Setting output: targetCommitSha: b2242877cacb038a31c8c6a5d9d32f6df0ecaed9
Setting output: targetBranch: v1-10-test

This should cause much more predictable builds for "non-master-branch" PRs and direct pushes (scripts from the target branch are used to build images in "workflow_run" Build Image runs ) .

@potiuk potiuk force-pushed the use-v1-10-scripts-in-v1-10-pulls-push branch from 040222b to 229a951 Compare November 13, 2020 12:54
@potiuk
Copy link
Member Author

potiuk commented Nov 13, 2020 •
edited
Loading

One small update @kaxil -> i noticed that the Job name overriding the scripts still mentioned master, now it will tell exactly from which branch the scripts are used.

potiuk
potiuk commented Nov 13, 2020
View reviewed changes
.github/workflows/build-images-workflow-run.yml Outdated
Copy link
Member Author

@potiuk potiuk Nov 13, 2020

Choose a reason for hiding this comment

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

Here we tell exactly which version of the scripts is used to build the images @kaxil

Copy link
Member

@kaxil kaxil Nov 13, 2020

Choose a reason for hiding this comment

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

👍

@potiuk
Copy link
Member Author

potiuk commented Nov 13, 2020

Right:

Screenshot from 2020-11-13 14-06-35

@potiuk
Copy link
Member Author

potiuk commented Nov 13, 2020 •
edited
Loading

And checkout still has "master" (just in the job name) .

@kaxil
Copy link
Member

kaxil commented Nov 13, 2020

And checkout still has "master" (just in the job name) .

eeh yeah, can we update that

@potiuk
Copy link
Member Author

potiuk commented Nov 13, 2020

And checkout still has "master" (just in the job name) .

eeh yeah, can we update that

absolutely!

@potiuk potiuk force-pushed the use-v1-10-scripts-in-v1-10-pulls-push branch from 229a951 to 86eefec Compare November 13, 2020 13:10
@potiuk
Copy link
Member Author

potiuk commented Nov 13, 2020

Screenshot from 2020-11-13 14-13-54

Good Enough :)

@potiuk
For v1-10-test PRs and pushes, use target branch scripts for images
cd0d1b2 
Previously, always master scripts were used to build images
for workflow_run, because workflow_run always runs from master
branch. However that causes some surprising effects becuase the
sripts from master had to support both master and 1.10.

This change utilises a new feature in the "get-workflow-origin"
action - to get the target branch of PR and uses ci scripts from that
target branch.

This is perfectly secure, because both v1-10-test, v1-10-stable
and future 2-0 branches can only be updated by committers,
either by direct push or by merge.
@potiuk potiuk force-pushed the use-v1-10-scripts-in-v1-10-pulls-push branch from 86eefec to cd0d1b2 Compare November 13, 2020 13:18
@potiuk potiuk mentioned this pull request Nov 13, 2020
Merged
@potiuk potiuk merged commit 7c4fe19 into apache:master Nov 13, 2020
@potiuk potiuk deleted the use-v1-10-scripts-in-v1-10-pulls-push branch November 13, 2020 13:28
potiuk added a commit that referenced this pull request Nov 14, 2020
@potiuk
For v1-10-test PRs and pushes, use target branch scripts for images (#…
5ffeab6 
…12339)

Previously, always master scripts were used to build images
for workflow_run, because workflow_run always runs from master
branch. However that causes some surprising effects becuase the
sripts from master had to support both master and 1.10.

This change utilises a new feature in the "get-workflow-origin"
action - to get the target branch of PR and uses ci scripts from that
target branch.

This is perfectly secure, because both v1-10-test, v1-10-stable
and future 2-0 branches can only be updated by committers,
either by direct push or by merge.

(cherry picked from commit 7c4fe19)
@potiuk potiuk added this to the Airflow 1.10.13 milestone Nov 14, 2020
@potiuk potiuk added the type:misc/internal Changelog: Misc changes that should appear in change log label Nov 14, 2020
potiuk added a commit that referenced this pull request Nov 16, 2020
@potiuk
For v1-10-test PRs and pushes, use target branch scripts for images (#…
c4a0b2a 
…12339)

Previously, always master scripts were used to build images
for workflow_run, because workflow_run always runs from master
branch. However that causes some surprising effects becuase the
sripts from master had to support both master and 1.10.

This change utilises a new feature in the "get-workflow-origin"
action - to get the target branch of PR and uses ci scripts from that
target branch.

This is perfectly secure, because both v1-10-test, v1-10-stable
and future 2-0 branches can only be updated by committers,
either by direct push or by merge.

(cherry picked from commit 7c4fe19)
potiuk added a commit that referenced this pull request Nov 16, 2020
@potiuk
For v1-10-test PRs and pushes, use target branch scripts for images (#…
ca7cc59 
…12339)

Previously, always master scripts were used to build images
for workflow_run, because workflow_run always runs from master
branch. However that causes some surprising effects becuase the
sripts from master had to support both master and 1.10.

This change utilises a new feature in the "get-workflow-origin"
action - to get the target branch of PR and uses ci scripts from that
target branch.

This is perfectly secure, because both v1-10-test, v1-10-stable
and future 2-0 branches can only be updated by committers,
either by direct push or by merge.

(cherry picked from commit 7c4fe19)
kaxil pushed a commit that referenced this pull request Nov 18, 2020
@potiuk @kaxil
For v1-10-test PRs and pushes, use target branch scripts for images (#…
ee92e47 
…12339)

Previously, always master scripts were used to build images
for workflow_run, because workflow_run always runs from master
branch. However that causes some surprising effects becuase the
sripts from master had to support both master and 1.10.

This change utilises a new feature in the "get-workflow-origin"
action - to get the target branch of PR and uses ci scripts from that
target branch.

This is perfectly secure, because both v1-10-test, v1-10-stable
and future 2-0 branches can only be updated by committers,
either by direct push or by merge.

(cherry picked from commit 7c4fe19)
cfei18 pushed a commit to cfei18/incubator-airflow that referenced this pull request Mar 5, 2021
@potiuk
For v1-10-test PRs and pushes, use target branch scripts for images (a…
1b55612 
…pache#12339)

Previously, always master scripts were used to build images
for workflow_run, because workflow_run always runs from master
branch. However that causes some surprising effects becuase the
sripts from master had to support both master and 1.10.

This change utilises a new feature in the "get-workflow-origin"
action - to get the target branch of PR and uses ci scripts from that
target branch.

This is perfectly secure, because both v1-10-test, v1-10-stable
and future 2-0 branches can only be updated by committers,
either by direct push or by merge.

(cherry picked from commit 7c4fe19)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kaxil kaxil kaxil approved these changes

@ashb ashb ashb approved these changes

@mik-laj mik-laj Awaiting requested review from mik-laj

@turbaszek turbaszek Awaiting requested review from turbaszek

Assignees

No one assigned

Labels

area:dev-tools full tests needed We need to run full set of tests for this PR to merge type:misc/internal Changelog: Misc changes that should appear in change log

Projects

None yet

Milestone

Airflow 1.10.13

Development

Successfully merging this pull request may close these issues.

3 participants

@potiuk @kaxil @ashb