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/24 18:27:04 UTC
[GitHub] [airflow] potiuk opened a new pull request #13308: Adds LDAP "extra" dependencies to ldap provider.
potiuk opened a new pull request #13308:
URL: https://github.com/apache/airflow/pull/13308
Fixes #13306
<!--
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] potiuk commented on a change in pull request #13308: Adds missing "extra" dependencies to providers.
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #13308:
URL: https://github.com/apache/airflow/pull/13308#discussion_r548718476
##########
File path: setup.py
##########
@@ -766,9 +766,9 @@ def write_version(filename: str = os.path.join(*[my_dir, "airflow", "git_version
'jdbc': ["jdbc"],
'jenkins': ["jenkins"],
'jira': ["jira"],
- 'kerberos': [],
+ 'kerberos': ["kerberos"],
Review comment:
actually I remove back statsd/kerberos. We do not have neither of them as providers :)
Similarly there are no providers for dask, apache.atlas, rabbitmq, sentry. There is one for rableau but it correctly has no additional requirements.
So just correcting LDAP was right intiially. I removed stasd/kerberos.
----------------------------------------------------------------
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 #13308: Adds missing "extra" dependencies to providers.
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #13308:
URL: https://github.com/apache/airflow/pull/13308#issuecomment-750944713
[The Workflow run](https://github.com/apache/airflow/actions/runs/442829999) is cancelling this PR. Building images for the PR has failed. Follow the the workflow link to check the reason.
----------------------------------------------------------------
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 #13308: Adds missing python-ldap dependency to LDAP extra.
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #13308:
URL: https://github.com/apache/airflow/pull/13308#issuecomment-751227494
[The Workflow run](https://github.com/apache/airflow/actions/runs/443856888) is cancelling this PR. Building images for the PR has failed. Follow the the workflow link to check the reason.
----------------------------------------------------------------
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 #13308: Adds missing python-ldap dependency to LDAP extra.
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #13308:
URL: https://github.com/apache/airflow/pull/13308#issuecomment-751246732
[The Workflow run](https://github.com/apache/airflow/actions/runs/444067692) 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] potiuk commented on a change in pull request #13308: Adds missing "extra" dependencies to providers.
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #13308:
URL: https://github.com/apache/airflow/pull/13308#discussion_r548703764
##########
File path: setup.py
##########
@@ -766,9 +766,9 @@ def write_version(filename: str = os.path.join(*[my_dir, "airflow", "git_version
'jdbc': ["jdbc"],
'jenkins': ["jenkins"],
'jira': ["jira"],
- 'kerberos': [],
+ 'kerberos': ["kerberos"],
Review comment:
I will review it fully,
----------------------------------------------------------------
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 #13308: Adds missing python-ldap dependency to LDAP extra.
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #13308:
URL: https://github.com/apache/airflow/pull/13308#issuecomment-751705997
Hey @kaxil -> I'd love to merge this one as it changes the image quite significantly at early stage and I wanted to refresh the CI images right after. If you are on holidays, I will merge it with @mik-laj approval - your comments have been applied.
----------------------------------------------------------------
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 #13308: Adds missing python-ldap dependency to LDAP extra.
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #13308:
URL: https://github.com/apache/airflow/pull/13308#issuecomment-751002783
[The Workflow run](https://github.com/apache/airflow/actions/runs/442928530) is cancelling this PR. Building images for the PR has failed. Follow the the workflow link to check the reason.
----------------------------------------------------------------
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 #13308: Adds missing python-ldap dependency to LDAP extra.
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #13308:
URL: https://github.com/apache/airflow/pull/13308#discussion_r548737911
##########
File path: setup.py
##########
@@ -766,9 +766,9 @@ def write_version(filename: str = os.path.join(*[my_dir, "airflow", "git_version
'jdbc': ["jdbc"],
'jenkins': ["jenkins"],
'jira': ["jira"],
- 'kerberos': [],
+ 'kerberos': ["kerberos"],
Review comment:
Seems that we simply miss `python-ldap` dependency (and it goes as far as 1.10.4 https://issues.apache.org/jira/browse/AIRFLOW-5261 and https://apache-airflow.slack.com/archives/C0146STM600/p1608836687492700
##########
File path: setup.py
##########
@@ -766,9 +766,9 @@ def write_version(filename: str = os.path.join(*[my_dir, "airflow", "git_version
'jdbc': ["jdbc"],
'jenkins': ["jenkins"],
'jira': ["jira"],
- 'kerberos': [],
+ 'kerberos': ["kerberos"],
Review comment:
🤦 🤦 . There is also no `ldap` provider either :(. This is a different issue altogether
----------------------------------------------------------------
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 #13308: Adds missing python-ldap dependency to LDAP extra.
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #13308:
URL: https://github.com/apache/airflow/pull/13308#issuecomment-751284240
> I am thinking of a code similar to the following.
>
> ```python
> subprocess.run(['docker', '--rm', $DOCKER_IMAGE, 'python', '-c', 'import ldap'])
> ```
I do not see a reason we should treat it differently. If we add it, we should add it for all packages not only for this library - we have plenty of other packages and possibly we should add them all this way. But that should be a separate change where we identify all such libraries and check if they can be imported/used? Maybe just create an issue for it with all such libraries. Maybe you can creat an issue for it ?
UPDATE: I added an issue: #13315
----------------------------------------------------------------
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 #13308: Adds missing python-ldap dependency to LDAP extra.
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #13308:
URL: https://github.com/apache/airflow/pull/13308#discussion_r548732125
##########
File path: setup.py
##########
@@ -766,9 +766,9 @@ def write_version(filename: str = os.path.join(*[my_dir, "airflow", "git_version
'jdbc': ["jdbc"],
'jenkins': ["jenkins"],
'jira': ["jira"],
- 'kerberos': [],
+ 'kerberos': ["kerberos"],
Review comment:
🤦 🤦 . There is no `ldap` provider either :(. I thougght about IMAP when I thought it is missing. This is a different issue altogether
----------------------------------------------------------------
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 #13308: Adds missing python-ldap dependency to LDAP extra.
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #13308:
URL: https://github.com/apache/airflow/pull/13308#issuecomment-751202522
[The Workflow run](https://github.com/apache/airflow/actions/runs/443699682) is cancelling this PR. Building images for the PR has failed. Follow the the workflow link to check the reason.
----------------------------------------------------------------
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] mik-laj commented on pull request #13308: Adds missing python-ldap dependency to LDAP extra.
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #13308:
URL: https://github.com/apache/airflow/pull/13308#issuecomment-751251261
Should we add tests to detect if the library has installed correctly? As far as I can see, it has system dependencies, so there is a chance that updating the base image could cause problems that we could easily overlook.
I am thinking of a code similar to the following.
```python
subprocess.run(['docker', '--rm', $DOCKER_IMAGE, 'python', '-c', 'import ldap'])
```
----------------------------------------------------------------
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 #13308: Adds missing python-ldap dependency to LDAP extra.
Posted by GitBox <gi...@apache.org>.
potiuk merged pull request #13308:
URL: https://github.com/apache/airflow/pull/13308
----------------------------------------------------------------
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 #13308: Adds missing LDAP "extra" dependencies to LDAP provider.
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #13308:
URL: https://github.com/apache/airflow/pull/13308#discussion_r548718476
##########
File path: setup.py
##########
@@ -766,9 +766,9 @@ def write_version(filename: str = os.path.join(*[my_dir, "airflow", "git_version
'jdbc': ["jdbc"],
'jenkins': ["jenkins"],
'jira': ["jira"],
- 'kerberos': [],
+ 'kerberos': ["kerberos"],
Review comment:
actually I remove back statsd/kerberos. We do not have neither of them as providers :)
Similarly there are no providers for dask, apache.atlas, rabbitmq, sentry. There is one for tableau but it correctly has no additional requirements.
So just correcting LDAP was right intiially. I removed stasd/kerberos.
----------------------------------------------------------------
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 #13308: Adds missing python-ldap dependency to LDAP extra.
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #13308:
URL: https://github.com/apache/airflow/pull/13308#issuecomment-751284240
> I am thinking of a code similar to the following.
>
> ```python
> subprocess.run(['docker', '--rm', $DOCKER_IMAGE, 'python', '-c', 'import ldap'])
> ```
I do not see a reason we should treat it differently. If we add it, we should add it for all packages not only for this library - we have plenty of other packages and possibly we should add them all this way. But that should be a separate change where we identify all such libraries and check if they can be imported/used? Maybe just create an issue for it with all such libraries. Maybe you can creat an issue for 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] github-actions[bot] commented on pull request #13308: Adds missing python-ldap dependency to LDAP extra.
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #13308:
URL: https://github.com/apache/airflow/pull/13308#issuecomment-751309510
[The Workflow run](https://github.com/apache/airflow/actions/runs/444806376) is cancelling this PR. Building images for the PR has failed. Follow the the workflow link to check the reason.
----------------------------------------------------------------
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 #13308: Adds missing python-ldap dependency to LDAP extra.
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #13308:
URL: https://github.com/apache/airflow/pull/13308#issuecomment-751365231
Looks like it could be merged :)
----------------------------------------------------------------
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 #13308: Adds missing LDAP "extra" dependencies to LDAP provider.
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #13308:
URL: https://github.com/apache/airflow/pull/13308#discussion_r548725115
##########
File path: setup.py
##########
@@ -766,9 +766,9 @@ def write_version(filename: str = os.path.join(*[my_dir, "airflow", "git_version
'jdbc': ["jdbc"],
'jenkins': ["jenkins"],
'jira': ["jira"],
- 'kerberos': [],
+ 'kerberos': ["kerberos"],
Review comment:
In our case extras <> providers. We have quite a few more extras than providers . We need to refactor setup.py badly to reflect that because it is awfully confusing (includig myself as you see) . We already discussed it in #12548 and I am going to do this very thing before New Year. I created #13309 to not forget about 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 commented on pull request #13308: Adds missing python-ldap dependency to LDAP extra.
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #13308:
URL: https://github.com/apache/airflow/pull/13308#issuecomment-751242718
Hey @kaxil I think this one should succeed :)
----------------------------------------------------------------
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 #13308: Adds missing python-ldap dependency to LDAP extra.
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #13308:
URL: https://github.com/apache/airflow/pull/13308#issuecomment-750953588
[The Workflow run](https://github.com/apache/airflow/actions/runs/442910179) is cancelling this PR. Building images for the PR has failed. Follow the the workflow link to check the reason.
----------------------------------------------------------------
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 #13308: Adds missing "extra" dependencies to providers.
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #13308:
URL: https://github.com/apache/airflow/pull/13308#discussion_r548697015
##########
File path: setup.py
##########
@@ -766,9 +766,9 @@ def write_version(filename: str = os.path.join(*[my_dir, "airflow", "git_version
'jdbc': ["jdbc"],
'jenkins': ["jenkins"],
'jira': ["jira"],
- 'kerberos': [],
+ 'kerberos': ["kerberos"],
Review comment:
Jarek, looks like `apache.beam` one is also empty (Line 718) -- should it be empty, it should have `apache.beam` right?
Similarly I am not sure if the following should be empty too:
- dask
- apache.atlas
- rabbitmq
- sentry
- tableau
WDYT ?
----------------------------------------------------------------
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 #13308: Adds missing LDAP "extra" dependencies to LDAP provider.
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #13308:
URL: https://github.com/apache/airflow/pull/13308#discussion_r548725115
##########
File path: setup.py
##########
@@ -766,9 +766,9 @@ def write_version(filename: str = os.path.join(*[my_dir, "airflow", "git_version
'jdbc': ["jdbc"],
'jenkins': ["jenkins"],
'jira': ["jira"],
- 'kerberos': [],
+ 'kerberos': ["kerberos"],
Review comment:
In our case extras <> providers. We have quite a few more extras than providers . We need to refactor setup.py badly to reflect that because it is awfully confusing (I was confused myself as you see) . We already discussed it in #12548 and I am going to do this very thing before New Year. I created #13309 to not forget about 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 commented on pull request #13308: Adds missing python-ldap dependency to LDAP extra.
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #13308:
URL: https://github.com/apache/airflow/pull/13308#issuecomment-751521091
> Should we add tests to detect if the library has installed correctly? As far as I can see, it has system dependencies, so there is a chance that updating the base image could cause problems that we could easily overlook.
>
> I am thinking of a code similar to the following.
>
> ```python
> subprocess.run(['docker', '--rm', $DOCKER_IMAGE, 'python', '-c', 'import ldap'])
> ```
I rebased the change on top of #13329 which re-enables more comprehensive verification (including checking imports for all added features). I've added LDAP check there.
----------------------------------------------------------------
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 #13308: Adds missing "extra" dependencies to providers.
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #13308:
URL: https://github.com/apache/airflow/pull/13308#issuecomment-750943569
> Looks like `kerberos` and `statsd` are also missing, can you add them too in this PR and rename it too "Adds missing extra dependencies" ?
Done.
----------------------------------------------------------------
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 #13308: Adds missing python-ldap dependency to LDAP extra.
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #13308:
URL: https://github.com/apache/airflow/pull/13308#issuecomment-751519991
Hey @kaxil - would love to merge it and get the images rebuilt (mostly from scratch for the ldap dependency).
----------------------------------------------------------------
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 #13308: Adds missing LDAP "extra" dependencies to LDAP provider.
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #13308:
URL: https://github.com/apache/airflow/pull/13308#discussion_r548732125
##########
File path: setup.py
##########
@@ -766,9 +766,9 @@ def write_version(filename: str = os.path.join(*[my_dir, "airflow", "git_version
'jdbc': ["jdbc"],
'jenkins': ["jenkins"],
'jira': ["jira"],
- 'kerberos': [],
+ 'kerberos': ["kerberos"],
Review comment:
🤦 🤦 . There is also no `ldap` provider :(. This is a different issue altogether
----------------------------------------------------------------
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