You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2020/11/13 12:36:21 UTC
[GitHub] [airflow] potiuk opened a new pull request #12339: For v1-10-test PRs and pushes, use target branch scripts for images
potiuk opened a new pull request #12339:
URL: https://github.com/apache/airflow/pull/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.
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/master/UPDATING.md).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on pull request #12339: For v1-10-test PRs and pushes, use target branch scripts for images
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #12339:
URL: https://github.com/apache/airflow/pull/12339#issuecomment-726755794
> And checkout still has "master" (just in the job name) .
eeh yeah, can we update that
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #12339: For v1-10-test PRs and pushes, use target branch scripts for images
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #12339:
URL: https://github.com/apache/airflow/pull/12339#discussion_r522933005
##########
File path: .github/workflows/build-images-workflow-run.yml
##########
@@ -321,13 +322,16 @@ jobs:
uses: actions/checkout@v2
with:
path: "main-airflow"
+ ref: "${{ needs.cancel-workflow-runs.outputs.targetBranch }}"
if: steps.defaults.outputs.proceed == 'true'
- name: "Setup python"
uses: actions/setup-python@v2
with:
python-version: ${{ needs.build-info.outputs.defaultPythonVersion }}
if: steps.defaults.outputs.proceed == 'true'
- - name: "Override 'scripts/ci' with the ${{ github.ref }} version so that the PR cannot override it."
+ - name: |
Review comment:
Here we tell exactly which version of the scripts is used to build the images @kaxil
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk edited a comment on pull request #12339: For v1-10-test PRs and pushes, use target branch scripts for images
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #12339:
URL: https://github.com/apache/airflow/pull/12339#issuecomment-726742996
Testing it here:
Direct push to master: https://github.com/potiuk/airflow/actions/runs/361589802
PR: from a fork to master: https://github.com/potiuk/airflow/actions/runs/361596665
Direct push to v1-10-test (stable will work the same): https://github.com/potiuk/airflow/actions/runs/361597050
PR from a fork to v1-10-test (stable will work the same): https://github.com/potiuk/airflow/actions/runs/361597777
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 ) .
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk merged pull request #12339: For v1-10-test PRs and pushes, use target branch scripts for images
Posted by GitBox <gi...@apache.org>.
potiuk merged pull request #12339:
URL: https://github.com/apache/airflow/pull/12339
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #12339: For v1-10-test PRs and pushes, use target branch scripts for images
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #12339:
URL: https://github.com/apache/airflow/pull/12339#issuecomment-726742996
Testing it here:
Direct push: https://github.com/potiuk/airflow/actions/runs/361589802
PR: from a fork: https://github.com/potiuk/airflow/actions/runs/361590584
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 ) .
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #12339: For v1-10-test PRs and pushes, use target branch scripts for images
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #12339:
URL: https://github.com/apache/airflow/pull/12339#discussion_r522939242
##########
File path: .github/workflows/build-images-workflow-run.yml
##########
@@ -321,13 +322,16 @@ jobs:
uses: actions/checkout@v2
with:
path: "main-airflow"
+ ref: "${{ needs.cancel-workflow-runs.outputs.targetBranch }}"
if: steps.defaults.outputs.proceed == 'true'
- name: "Setup python"
uses: actions/setup-python@v2
with:
python-version: ${{ needs.build-info.outputs.defaultPythonVersion }}
if: steps.defaults.outputs.proceed == 'true'
- - name: "Override 'scripts/ci' with the ${{ github.ref }} version so that the PR cannot override it."
+ - name: |
Review comment:
👍
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk edited a comment on pull request #12339: For v1-10-test PRs and pushes, use target branch scripts for images
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #12339:
URL: https://github.com/apache/airflow/pull/12339#issuecomment-726742996
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 ) .
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #12339: For v1-10-test PRs and pushes, use target branch scripts for images
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #12339:
URL: https://github.com/apache/airflow/pull/12339#issuecomment-726755905
> > And checkout still has "master" (just in the job name) .
>
> eeh yeah, can we update that
absolutely!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk edited a comment on pull request #12339: For v1-10-test PRs and pushes, use target branch scripts for images
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #12339:
URL: https://github.com/apache/airflow/pull/12339#issuecomment-726742996
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
```
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
```
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 ) .
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk edited a comment on pull request #12339: For v1-10-test PRs and pushes, use target branch scripts for images
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #12339:
URL: https://github.com/apache/airflow/pull/12339#issuecomment-726755272
And checkout still has "master" (just in the job name) .
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #12339: For v1-10-test PRs and pushes, use target branch scripts for images
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #12339:
URL: https://github.com/apache/airflow/pull/12339#issuecomment-726742572
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!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk edited a comment on pull request #12339: For v1-10-test PRs and pushes, use target branch scripts for images
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #12339:
URL: https://github.com/apache/airflow/pull/12339#issuecomment-726749145
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #12339: For v1-10-test PRs and pushes, use target branch scripts for images
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #12339:
URL: https://github.com/apache/airflow/pull/12339#issuecomment-726758430
Good Enough :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #12339: For v1-10-test PRs and pushes, use target branch scripts for images
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #12339:
URL: https://github.com/apache/airflow/pull/12339#issuecomment-726749145
One small update @kaxil -> i noticed that the Job name overriding the scripts still mentioned master, now it will exactly from which branch the scripts are used.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #12339: For v1-10-test PRs and pushes, use target branch scripts for images
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #12339:
URL: https://github.com/apache/airflow/pull/12339#issuecomment-726754711
Right:
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #12339: For v1-10-test PRs and pushes, use target branch scripts for images
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #12339:
URL: https://github.com/apache/airflow/pull/12339#issuecomment-726755272
And checkout still has "master" :(.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk edited a comment on pull request #12339: For v1-10-test PRs and pushes, use target branch scripts for images
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #12339:
URL: https://github.com/apache/airflow/pull/12339#issuecomment-726742996
Testing it here:
Direct push: https://github.com/potiuk/airflow/actions/runs/361589802
PR: from a fork to master: https://github.com/potiuk/airflow/actions/runs/361596665
PR from same repo to v1-10-test (stable will work the same): https://github.com/potiuk/airflow/actions/runs/361597050
PR from a fork to v1-10-test (stable will work the same): https://github.com/potiuk/airflow/actions/runs/361597777
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 ) .
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org