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 2022/07/02 14:55:40 UTC
[GitHub] [airflow] potiuk opened a new pull request, #24802: Unified "dash-name" convention for outputs in ci workflows.
potiuk opened a new pull request, #24802:
URL: https://github.com/apache/airflow/pull/24802
There were errors with retieving constraints branch caused by
using different convention for output names (sometimes dash,
sometimes camelCase as suggested by most GitHub documents).
The "dash-name" looks much better and is far more readable so
we shoud unify all internal outputs to follow it
<!--
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/main/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 a newsfragement file, named `{pr_number}.significant.rst`, in [newsfragments](https://github.com/apache/airflow/tree/main/newsfragments).
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk merged pull request #24802: Unified "dash-name" convention for outputs in ci workflows.
Posted by GitBox <gi...@apache.org>.
potiuk merged PR #24802:
URL: https://github.com/apache/airflow/pull/24802
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #24802: Unified "dash-name" convention for outputs in ci workflows.
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #24802:
URL: https://github.com/apache/airflow/pull/24802#discussion_r912395048
##########
.github/workflows/build-images.yml:
##########
@@ -104,109 +104,103 @@ jobs:
ref: ${{ env.TARGET_COMMIT_SHA }}
persist-credentials: false
fetch-depth: 2
- - name: "Checkout ${{ github.ref }} ( ${{ github.sha }} )"
- uses: actions/checkout@v3
- with:
- persist-credentials: false
- submodules: recursive
- name: "Setup python"
uses: actions/setup-python@v4
with:
# We do not have output from selective checks yet, so we need to hardcode python
python-version: 3.7
cache: 'pip'
cache-dependency-path: ./dev/breeze/setup*
+ - name: "Retrieve defaults from branch_defaults.py"
+ # We cannot "execute" the branch_defaults.py python code here because that would be
+ # a security problem (we cannot run any code that comes from the sources coming from the PR.
+ # Therefore, we extract the branches via embedded Python code
+ # we need to do it before next step replaces checked-out breeze and scripts code coming from
+ # the PR, because the PR defaults have to be retrieved here.
+ id: defaults
+ run: |
+ python - <<EOF >>$GITHUB_ENV
+ from pathlib import Path
+ import re
+ import sys
+
+ DEFAULTS_CONTENT = Path('dev/breeze/src/airflow_breeze/branch_defaults.py').read_text()
+ BRANCH_PATTERN = r'^AIRFLOW_BRANCH = "(.*)"$'
+ CONSTRAINTS_BRANCH_PATTERN = r'^DEFAULT_AIRFLOW_CONSTRAINTS_BRANCH = "(.*)"$'
+ DEBIAN_VERSION_PATTERN = r'^DEBIAN_VERSION = "(.*)"$'
+
+ branch = re.search(BRANCH_PATTERN, DEFAULTS_CONTENT, re.MULTILINE).group(1)
+ constraints_branch = re.search(CONSTRAINTS_BRANCH_PATTERN, DEFAULTS_CONTENT, re.MULTILINE).group(1)
+ debian_version = re.search(DEBIAN_VERSION_PATTERN, DEFAULTS_CONTENT, re.MULTILINE).group(1)
+
+ output = f"""
+ DEFAULT_BRANCH={branch}
+ DEFAULT_CONSTRAINTS_BRANCH={constraints_branch}
+ DEBIAN_VERSION={debian_version}
+ """.strip()
+
+ print(output)
+ # Stdout is redirected to GITHUB_ENV but we also print it to stderr to see it in ci log
+ print(output, file=sys.stderr)
+ EOF
+ - name: Checkout main branch to 'main-airflow' folder to use breeze from there.
Review Comment:
Right :). copy paste. I will also add explicit "main" ref there - this is always used by default with pull-request-target, but, well, explicit is better than implicit, especially in security-related context.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a diff in pull request #24802: Unified "dash-name" convention for outputs in ci workflows.
Posted by GitBox <gi...@apache.org>.
mik-laj commented on code in PR #24802:
URL: https://github.com/apache/airflow/pull/24802#discussion_r912383935
##########
.github/workflows/build-images.yml:
##########
@@ -104,109 +104,103 @@ jobs:
ref: ${{ env.TARGET_COMMIT_SHA }}
persist-credentials: false
fetch-depth: 2
- - name: "Checkout ${{ github.ref }} ( ${{ github.sha }} )"
- uses: actions/checkout@v3
- with:
- persist-credentials: false
- submodules: recursive
- name: "Setup python"
uses: actions/setup-python@v4
with:
# We do not have output from selective checks yet, so we need to hardcode python
python-version: 3.7
cache: 'pip'
cache-dependency-path: ./dev/breeze/setup*
+ - name: "Retrieve defaults from branch_defaults.py"
+ # We cannot "execute" the branch_defaults.py python code here because that would be
+ # a security problem (we cannot run any code that comes from the sources coming from the PR.
+ # Therefore, we extract the branches via embedded Python code
+ # we need to do it before next step replaces checked-out breeze and scripts code coming from
+ # the PR, because the PR defaults have to be retrieved here.
+ id: defaults
+ run: |
+ python - <<EOF >>$GITHUB_ENV
+ from pathlib import Path
+ import re
+ import sys
+
+ DEFAULTS_CONTENT = Path('dev/breeze/src/airflow_breeze/branch_defaults.py').read_text()
+ BRANCH_PATTERN = r'^AIRFLOW_BRANCH = "(.*)"$'
+ CONSTRAINTS_BRANCH_PATTERN = r'^DEFAULT_AIRFLOW_CONSTRAINTS_BRANCH = "(.*)"$'
+ DEBIAN_VERSION_PATTERN = r'^DEBIAN_VERSION = "(.*)"$'
+
+ branch = re.search(BRANCH_PATTERN, DEFAULTS_CONTENT, re.MULTILINE).group(1)
+ constraints_branch = re.search(CONSTRAINTS_BRANCH_PATTERN, DEFAULTS_CONTENT, re.MULTILINE).group(1)
+ debian_version = re.search(DEBIAN_VERSION_PATTERN, DEFAULTS_CONTENT, re.MULTILINE).group(1)
+
+ output = f"""
+ DEFAULT_BRANCH={branch}
+ DEFAULT_CONSTRAINTS_BRANCH={constraints_branch}
+ DEBIAN_VERSION={debian_version}
+ """.strip()
+
+ print(output)
+ # Stdout is redirected to GITHUB_ENV but we also print it to stderr to see it in ci log
+ print(output, file=sys.stderr)
+ EOF
+ - name: Checkout main branch to 'main-airflow' folder to use breeze from there.
Review Comment:
The description does not match what this action actually does.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org