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/12/05 18:34:43 UTC
[GitHub] [airflow] ashb opened a new pull request #12842: Build normal/backport providers based on correct list
ashb opened a new pull request #12842:
URL: https://github.com/apache/airflow/pull/12842
This was using the backport list when trying to build the normal list,
leading to papermill package not getting built.
I have re-introduced the exit removed in #12841, and also done a little
bit of drive-by-tidying
<!--
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] ashb commented on pull request #12842: Build normal/backport providers based on correct list
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #12842:
URL: https://github.com/apache/airflow/pull/12842#issuecomment-739478099
Argh, I think because I created this fork from our master branch (whoops) something has gone on odd here.
----------------------------------------------------------------
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] ashb commented on pull request #12842: Build normal/backport providers based on correct list
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #12842:
URL: https://github.com/apache/airflow/pull/12842#issuecomment-739334563
Oh lol.
> ERROR! Some folders from providers package are not defined
> Please add them to dev/provider_packages/prepare_provider_packages.py:
I think that's now failing _exactly because_ papermill is not backportable.
----------------------------------------------------------------
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] ashb commented on pull request #12842: Build normal/backport providers based on correct list
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #12842:
URL: https://github.com/apache/airflow/pull/12842#issuecomment-739335605
> > I think that's now failing _exactly because_ papermill is not backportable.
>
> Nope. I thin this is the changed sed.
```
ERROR! Some folders from providers package are not defined
Please add them to dev/provider_packages/prepare_provider_packages.py:
papermill.operators
```
----------------------------------------------------------------
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] ashb edited a comment on pull request #12842: Build normal/backport providers based on correct list
Posted by GitBox <gi...@apache.org>.
ashb edited a comment on pull request #12842:
URL: https://github.com/apache/airflow/pull/12842#issuecomment-739419341
Hmmm 60 vs 61 now.
61 looks like the "right" value too
```ipython console
In [2]: len(setup.PROVIDERS_REQUIREMENTS)
Out[2]: 61
```
----------------------------------------------------------------
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 #12842: Build normal/backport providers based on correct list
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #12842:
URL: https://github.com/apache/airflow/pull/12842#issuecomment-739335427
> I think that's now failing _exactly because_ papermill is not backportable.
Nope. I thin this is the changed sed. It did not even hit the changed it yet. The few lines above I am finding all the directories that have some hooks/operators etc. in and turn them into provider list with the sed and then remove all the providers returned by "all providers" - > at the end the list should be empty and it is not.
----------------------------------------------------------------
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 #12842: Build normal/backport providers based on correct list
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #12842:
URL: https://github.com/apache/airflow/pull/12842#issuecomment-739334395
[The Workflow run](https://github.com/apache/airflow/actions/runs/402917006) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.
----------------------------------------------------------------
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 #12842: Build normal/backport providers based on correct list
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #12842:
URL: https://github.com/apache/airflow/pull/12842#issuecomment-739332721
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.
----------------------------------------------------------------
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 #12842: Build normal/backport providers based on correct list
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #12842:
URL: https://github.com/apache/airflow/pull/12842#issuecomment-739364221
[The Workflow run](https://github.com/apache/airflow/actions/runs/402967051) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.
----------------------------------------------------------------
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 merged pull request #12842: Build normal/backport providers based on correct list
Posted by GitBox <gi...@apache.org>.
kaxil merged pull request #12842:
URL: https://github.com/apache/airflow/pull/12842
----------------------------------------------------------------
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] ashb commented on pull request #12842: Build normal/backport providers based on correct list
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #12842:
URL: https://github.com/apache/airflow/pull/12842#issuecomment-739332826
> Yours is nicer. I kill mine :)
Thanks, it took me rather longer though! (been poking at this for an hour)
----------------------------------------------------------------
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 #12842: Build normal/backport providers based on correct list
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #12842:
URL: https://github.com/apache/airflow/pull/12842#discussion_r536869364
##########
File path: scripts/in_container/run_prepare_provider_packages.sh
##########
@@ -24,7 +24,7 @@ LIST_OF_DIRS_FILE=$(mktemp)
cd "${AIRFLOW_SOURCES}/airflow/providers" || exit 1
-find . -type d | sed 's/.\///; s/\//\./g' | grep -E 'hooks|operators|sensors|secrets|utils' \
+find . -type d | sed 's!./!!; s!/!.!g' | grep -E 'hooks|operators|sensors|secrets|utils' \
Review comment:
Here I removed "./" in front and then replaced "/" with . to turn directories into providers "apache/hive" -> "apache.hive". That was the original meaning of this sed. I don't think the new sed does the same.
----------------------------------------------------------------
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] ashb commented on a change in pull request #12842: Build normal/backport providers based on correct list
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #12842:
URL: https://github.com/apache/airflow/pull/12842#discussion_r536849964
##########
File path: scripts/in_container/run_prepare_provider_packages.sh
##########
@@ -24,7 +24,7 @@ LIST_OF_DIRS_FILE=$(mktemp)
cd "${AIRFLOW_SOURCES}/airflow/providers" || exit 1
-find . -type d | sed 's/.\///; s/\//\./g' | grep -E 'hooks|operators|sensors|secrets|utils' \
+find . -type d | sed 's!./!!; s!/!.!g' | grep -E 'hooks|operators|sensors|secrets|utils' \
Review comment:
Slight tidy up -- can use different character than `/` for regex in sed, meaning `\/` isn't needed when dealing with paths/urls.
----------------------------------------------------------------
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] ashb commented on pull request #12842: Build normal/backport providers based on correct list
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #12842:
URL: https://github.com/apache/airflow/pull/12842#issuecomment-739478163
Yeah, Github has gotten confused. I'll re-open this PR from a new branch.
----------------------------------------------------------------
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] ashb commented on pull request #12842: Build normal/backport providers based on correct list
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #12842:
URL: https://github.com/apache/airflow/pull/12842#issuecomment-739332453
Lets see if this passes the provider test
----------------------------------------------------------------
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] ashb commented on pull request #12842: Build normal/backport providers based on correct list
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #12842:
URL: https://github.com/apache/airflow/pull/12842#issuecomment-739335693
(I ran non-backport locally, the sed works fine.)
----------------------------------------------------------------
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 #12842: Build normal/backport providers based on correct list
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #12842:
URL: https://github.com/apache/airflow/pull/12842#issuecomment-739335427
> I think that's now failing _exactly because_ papermill is not backportable.
Nope. I thin this is the changed sed.
----------------------------------------------------------------
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] ashb commented on pull request #12842: Build normal/backport providers based on correct list
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #12842:
URL: https://github.com/apache/airflow/pull/12842#issuecomment-739336021
K, so the issue was previously, the "list of providers" was always got from "normal", which we then checked against files on disk. We then always reset the list to backport.
Now with using the "right" list before the check, when building backports the list of providers didn't include papermill, so papermill was "left over" in the file, hence the error.
I've changed it to only check the list of providers vs files on disk in normal mode.
----------------------------------------------------------------
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] ashb commented on a change in pull request #12842: Build normal/backport providers based on correct list
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #12842:
URL: https://github.com/apache/airflow/pull/12842#discussion_r536849366
##########
File path: scripts/in_container/run_prepare_provider_packages.sh
##########
@@ -63,7 +72,6 @@ if [[ -z "$*" ]]; then
echo
exit 1
fi
- PROVIDER_PACKAGES=$(python3 "${PREPARE_PROVIDER_PACKAGES_PY}" list-backportable-packages)
Review comment:
This was the problem -- we were resetting this list to the backport list, even when building normal packages.
----------------------------------------------------------------
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 #12842: Build normal/backport providers based on correct list
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #12842:
URL: https://github.com/apache/airflow/pull/12842#issuecomment-739336225
Ah I see - that's why there was a "providers list" first and "backportable" later :). i see now that you got the same command for both 0-> my proposal was just replacing the second one :)
----------------------------------------------------------------
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] ashb commented on a change in pull request #12842: Build normal/backport providers based on correct list
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #12842:
URL: https://github.com/apache/airflow/pull/12842#discussion_r536849510
##########
File path: scripts/in_container/run_prepare_provider_packages.sh
##########
@@ -34,12 +34,21 @@ REFACTOR_PROVIDER_PACKAGES_PY="${AIRFLOW_SOURCES}/dev/provider_packages/refactor
verify_suffix_versions_for_package_preparation
+if [[ ${BACKPORT_PACKAGES} == "true" ]]; then
+ list_subcmd="list-backportable-packages"
+else
+ list_subcmd="list-providers-packages"
+fi
+
if [[ -z "$*" ]]; then
- PROVIDERS_PACKAGES=$(python3 "${PREPARE_PROVIDER_PACKAGES_PY}" list-providers-packages)
+ PROVIDER_PACKAGES=()
+ while IFS='' read -r line; do PROVIDER_PACKAGES+=("$line"); done < <(
+ python3 "${PREPARE_PROVIDER_PACKAGES_PY}" "$list_subcmd"
+ )
Review comment:
This is the style recommended by shell-check to convert lines in to a bash array.
----------------------------------------------------------------
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] ashb commented on pull request #12842: Build normal/backport providers based on correct list
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #12842:
URL: https://github.com/apache/airflow/pull/12842#issuecomment-739419341
Hmmm 60 vs 61 now.
----------------------------------------------------------------
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