Skip to content
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.

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

Adds missing python-ldap dependency to LDAP extra. #13308

Merged
merged 1 commit into from
Dec 28, 2020

Conversation

potiuk
Copy link
Member

@potiuk potiuk commented Dec 24, 2020

Please check only the last commit as this is based on #13329

It seems that for quite some time (1.10.4) the "ldap" extra
missed python-ldap dependency.

https://issues.apache.org/jira/browse/AIRFLOW-5261

Also LDAP seems to be popular enough to be added as default
extra in the production image.

Fixes #13306


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

@potiuk potiuk requested review from kaxil and mik-laj December 24, 2020 18:27
kaxil
kaxil previously requested changes Dec 24, 2020
Copy link
Member

@kaxil kaxil left a comment

Choose a reason for hiding this comment

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

Looks like kerberos and statsd are also missing, can you add them too in this PR and rename it too "Adds missing extra dependencies" ?

@potiuk potiuk changed the title Adds LDAP "extra" dependencies to ldap provider. Adds missing "extra" dependencies to ldap provider. Dec 24, 2020
@potiuk potiuk changed the title Adds missing "extra" dependencies to ldap provider. Adds missing "extra" dependencies to providers. Dec 24, 2020
@potiuk
Copy link
Member Author

potiuk commented Dec 24, 2020

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.

@github-actions
Copy link

The Workflow run is cancelling this PR. Building images for the PR has failed. Follow the the workflow link to check the reason.

setup.py Outdated Show resolved Hide resolved
@potiuk potiuk changed the title Adds missing "extra" dependencies to providers. Adds missing LDAP "extra" dependencies to LDAP provider. Dec 24, 2020
@potiuk potiuk changed the title Adds missing LDAP "extra" dependencies to LDAP provider. Adds missing python-ldap dependency to LDAP extra. Dec 24, 2020
@github-actions
Copy link

The Workflow run is cancelling this PR. Building images for the PR has failed. Follow the the workflow link to check the reason.

@potiuk potiuk self-assigned this Dec 24, 2020
@github-actions
Copy link

The Workflow run is cancelling this PR. Building images for the PR has failed. Follow the the workflow link to check the reason.

@potiuk potiuk force-pushed the adds-ldap-provider-missing-dependency branch from 84af944 to 2c27472 Compare December 25, 2020 07:34
@github-actions
Copy link

The Workflow run is cancelling this PR. Building images for the PR has failed. Follow the the workflow link to check the reason.

@potiuk potiuk force-pushed the adds-ldap-provider-missing-dependency branch from 2c27472 to 47ad00f Compare December 25, 2020 09:53
@github-actions
Copy link

The Workflow run is cancelling this PR. Building images for the PR has failed. Follow the the workflow link to check the reason.

@potiuk potiuk force-pushed the adds-ldap-provider-missing-dependency branch 3 times, most recently from 0597320 to 0c9a50a Compare December 25, 2020 12:27
@potiuk
Copy link
Member Author

potiuk commented Dec 25, 2020

Hey @kaxil I think this one should succeed :)

@github-actions
Copy link

The Workflow run 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*.

@mik-laj
Copy link
Member

mik-laj commented Dec 25, 2020

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.

subprocess.run(['docker', '--rm',  $DOCKER_IMAGE, 'python', '-c', 'import ldap'])

@potiuk
Copy link
Member Author

potiuk commented Dec 25, 2020

I am thinking of a code similar to the following.

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

@potiuk potiuk force-pushed the adds-ldap-provider-missing-dependency branch 2 times, most recently from ff8ed0d to eee5563 Compare December 25, 2020 20:58
@potiuk potiuk force-pushed the adds-ldap-provider-missing-dependency branch from eee5563 to f55846a Compare December 26, 2020 00:45
@github-actions
Copy link

The Workflow run is cancelling this PR. Building images for the PR has failed. Follow the the workflow link to check the reason.

@potiuk potiuk force-pushed the adds-ldap-provider-missing-dependency branch from f55846a to 55e3e19 Compare December 26, 2020 11:25
@potiuk potiuk requested a review from kaxil December 26, 2020 12:42
@potiuk
Copy link
Member Author

potiuk commented Dec 26, 2020

Looks like it could be merged :)

@potiuk potiuk force-pushed the adds-ldap-provider-missing-dependency branch from 55e3e19 to 3017ace Compare December 26, 2020 19:05
@potiuk
Copy link
Member Author

potiuk commented Dec 27, 2020

Hey @kaxil - would love to merge it and get the images rebuilt (mostly from scratch for the ldap dependency).

@potiuk potiuk force-pushed the adds-ldap-provider-missing-dependency branch from 3017ace to 5f9c282 Compare December 27, 2020 21:59
@potiuk
Copy link
Member Author

potiuk commented Dec 27, 2020

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.

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.

It seems that for quite some time (1.10.4) the "ldap" extra
missed python-ldap dependency.

https://issues.apache.org/jira/browse/AIRFLOW-5261

Also LDAP seems to be popular enough to be added as default
extra in the production image.

Fixes #13306
@potiuk potiuk force-pushed the adds-ldap-provider-missing-dependency branch from 5f9c282 to 20ea56c Compare December 28, 2020 13:03
@potiuk
Copy link
Member Author

potiuk commented Dec 28, 2020

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.

@potiuk potiuk dismissed kaxil’s stale review December 28, 2020 16:06

Need upgrading dockerfile with it.

@potiuk potiuk merged commit d23ac9b into master Dec 28, 2020
@potiuk potiuk added this to the Airflow 2.0.1 milestone Jan 2, 2021
@potiuk potiuk deleted the adds-ldap-provider-missing-dependency branch January 7, 2021 12:02
kaxil pushed a commit that referenced this pull request Jan 21, 2021
It seems that for quite some time (1.10.4) the "ldap" extra
missed python-ldap dependency.

https://issues.apache.org/jira/browse/AIRFLOW-5261

Also LDAP seems to be popular enough to be added as default
extra in the production image.

Fixes #13306

(cherry picked from commit d23ac9b)
kaxil pushed a commit to astronomer/airflow that referenced this pull request Apr 24, 2021
It seems that for quite some time (1.10.4) the "ldap" extra
missed python-ldap dependency.

https://issues.apache.org/jira/browse/AIRFLOW-5261

Also LDAP seems to be popular enough to be added as default
extra in the production image.

Fixes apache#13306

(cherry picked from commit d23ac9b)
(cherry picked from commit 047a197)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The "ldap" extra misses libldap dependency
3 participants