[AIRFLOW-1057] fix class name that is changed at docker package version 2.0.0 #2204

wasperen · 2017-03-30T13:54:53Z

Dear Airflow maintainers,

Please accept this PR. I understand that it will not be reviewed until I have checked off all the steps below!

JIRA

My PR addresses the following AIRFLOW-1057

Description

Here are some details about my PR, including screenshots of any UI changes:
This fixes an incorrect reference to a class in the docker package

Tests

My PR also changes the docker_operator test to check if the correct version of docker python is installed.

Commits

My commits all reference JIRA issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
1. Subject is separated from body by a blank line
2. Subject is limited to 50 characters
3. Subject does not end with a period
4. Subject uses the imperative mood ("add", not "adding")
5. Body wraps at 72 characters
6. Body explains "what" and "why", not "how"

bolkedebruin · 2017-03-30T22:02:43Z

Thanks for the contribution. Please follow the commit guidelines. Also, is this backward compatible?

wasperen · 2017-04-03T07:46:49Z

I have now improved the import to cater for those people that use docker-py prior to 2.0.0. It will now first try to import APIClient, but if that fails it imports Client as APIClient.

GaelMagnan · 2017-04-04T16:17:12Z

I've checked this pr it seems ok to me.

Maybe changing the require to be able to install either the docker-py (v1.xx) or docker (v2.xx) would be a good add.

wasperen · 2017-04-09T12:36:38Z

I am not familiar with the docker-py package. The package on pypi has not been updated since November 2016 and the documentation of that package seems to point to the docker package documentation (https://docker-py.readthedocs.io/en/latest/)

I have manually installed the docker package and did not rely on dependencies from setup.py. That's how I found out.

Can someone shed a light on the difference between these two packages?

GaelMagnan · 2017-04-09T20:57:43Z

They are basically the same package. Prior to version 2.x it was called docker-py, and since version 2 they switched to docker.
They actually have issues open on their project to make it more explicit because it's been causing a lot of confusion.

wasperen · 2017-04-10T07:18:14Z

Ok. I'll make the dependency such that it checks if either docker-py or docker is installed. Installing the docker dependency (in setup.py) will pull down the docker package.

wasperen · 2017-04-10T07:53:37Z

ok -- I know where to change the docker package dependency: line 125 of setup.py. Changing that to docker = ['docker>=2.0.0'] will get us install the package docker.

But I am not sure how to validate if docker-py may be installed and rely on that version. I am new to python packaging.

GaelMagnan · 2017-04-10T08:01:16Z

I'm not sure if it's considered good practice, but you can import it in a try except and if it's present it won't throw.

wasperen · 2017-04-10T15:32:41Z

that is now the way I do it. the import tries to import docker and use the APIClient class. If that fails it imports docker-py and use the class Client as APIClient.
I have also changed the dependency on the docker module rather than docker-py

GaelMagnan

It looks good to me, just the test maybe have the same comportment that in the operator, to handle the case where the docker-py library is installed.

GaelMagnan · 2017-04-20T09:02:19Z

tests/operators/docker_operator.py

 try:
    from airflow.operators.docker_operator import DockerOperator
-    from docker.client import Client
+    from docker.client import APIClient


You don't test if docker-py is installed during the tests to fallback?

codecov-io · 2017-04-26T08:44:29Z

Codecov Report

Merging #2204 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2204      +/-   ##
==========================================
+ Coverage   69.22%   69.24%   +0.02%     
==========================================
  Files         145      145              
  Lines       11134    11138       +4     
==========================================
+ Hits         7707     7712       +5     
+ Misses       3427     3426       -1

Impacted Files	Coverage Δ
airflow/operators/docker_operator.py	`97.5% <100%> (+0.13%)`	⬆️
airflow/models.py	`87.13% <0%> (+0.04%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fb21bcb...c3d73f8. Read the comment docs.

wasperen · 2017-04-26T11:12:23Z

updated the test so is does the same check on which docker package is installed.
I think this is now good to go?

bolkedebruin · 2017-05-12T09:44:10Z

please squash your commits, make sure you follow the commit guidelines for the commit message and then we are good to go.

wasperen · 2017-05-15T09:03:59Z

I think this is now in line with the guidelines -- anything I missed?

ghost · 2017-06-07T19:46:01Z

I think you are missing

commits all reference JIRA issues in their subject lines

so something like [AIRFLOW-1057] fixed class name that is changed at docker package version 2.0.0

wasperen · 2017-06-16T12:47:20Z

I think it already does have a subject line that is exactly that. Do you mean something else, @avigil-twist ?

ghost · 2017-06-18T05:37:29Z

you changed the name of this pull request but not the name of the commit itself

…on 2.0.0 adding conditional import to provide backward compatibility for package docker-py changed dependency for docker package; now docker rather than docker-py updated test cases to align to new docker class; also testing for old docker.py in test

wasperen · 2017-06-18T09:56:00Z

got it, @avigil-twist and fixed that now

russellpierce · 2017-12-05T03:52:48Z

This PR never got merged as far as I can tell, but the associated issue is marked as RESOLVED https://issues.apache.org/jira/browse/AIRFLOW-1057... is it actually RESOLVED?

wasperen · 2017-12-07T21:05:06Z

Well, @russellpierce, I must say I am not entirely sure either. I have been using my fork for most of the time. I'll check.

wasperen · 2017-12-07T21:17:29Z

Just had a quick look at the code (current master branch https://github.com/apache/incubator-airflow/blob/master/airflow/operators/docker_operator.py) but there we just import Client from docker. So nothing has changed there. I doubt whether this has been fixed then -- this still requires the docker module version < 2.0 to be installed...

I think I put the issue to "resolved" to mean: there is a pull request that needs validation. Reopened the issue.

ashb · 2017-12-09T15:43:28Z

airflow/operators/docker_operator.py

+    from docker import APIClient
+except ImportError:
+    # prior to version 2.0.0 of the module docker, the API client was called Client
+    from docker import Client as APIClient


Given we now depend upon the more recent docker lib (and that is the right thing to do) I don't think there is any need to have this fallback behaviour -- we will always have at least docker 2.0 won't we?

Sure. Maybe we should have that reflect in the setup.py as well? The master version still read "docker = ['docker-py>=1.6.0']" which now should be "docker = ['docker-py>=2.0.0']"

brokenjacobs · 2018-02-28T19:47:46Z

Has this stalled again or will it make it to 2.0?

Also FYI this pr doesn't work against docker >= 3.0.0 due to breaking api changes in the docker client library:
http://docker-py.readthedocs.io/en/stable/change-log.html

Fokko · 2018-02-28T20:44:26Z

@brokenjacobs This first needs to be fixed; there are a lot of merge conflicts right now.

gerardo · 2018-06-26T04:46:01Z

This PR can be closed. It was fixed on #3407

wasperen changed the title ~~fixed class name that is changed at docker package version 2.0.0~~ [AIRFLOW-1057] fixed class name that is changed at docker package version 2.0.0 Apr 13, 2017

GaelMagnan reviewed Apr 20, 2017

View reviewed changes

wasperen force-pushed the master branch from f22044e to 919dd9f Compare May 15, 2017 08:55

wasperen changed the title ~~[AIRFLOW-1057] fixed class name that is changed at docker package version 2.0.0~~ [AIRFLOW-1057] fix class name that is changed at docker package version 2.0.0 May 15, 2017

wasperen force-pushed the master branch from 919dd9f to 30d8b87 Compare May 15, 2017 09:02

wasperen force-pushed the master branch from 30d8b87 to c3d73f8 Compare June 18, 2017 09:54

ashb reviewed Dec 9, 2017

View reviewed changes

ashb closed this Nov 1, 2018

[AIRFLOW-1057] fix class name that is changed at docker package version 2.0.0 #2204

[AIRFLOW-1057] fix class name that is changed at docker package version 2.0.0 #2204

Uh oh!

Conversation

wasperen commented Mar 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

JIRA

Description

Tests

Commits

Uh oh!

bolkedebruin commented Mar 30, 2017

Uh oh!

wasperen commented Apr 3, 2017

Uh oh!

GaelMagnan commented Apr 4, 2017

Uh oh!

wasperen commented Apr 9, 2017

Uh oh!

GaelMagnan commented Apr 9, 2017

Uh oh!

wasperen commented Apr 10, 2017

Uh oh!

wasperen commented Apr 10, 2017

Uh oh!

GaelMagnan commented Apr 10, 2017

Uh oh!

wasperen commented Apr 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GaelMagnan left a comment

Choose a reason for hiding this comment

Uh oh!

GaelMagnan Apr 20, 2017

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Apr 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

wasperen commented Apr 26, 2017

Uh oh!

bolkedebruin commented May 12, 2017

Uh oh!

wasperen commented May 15, 2017

Uh oh!

ghost commented Jun 7, 2017

Uh oh!

wasperen commented Jun 16, 2017

Uh oh!

ghost commented Jun 18, 2017

Uh oh!

wasperen commented Jun 18, 2017

Uh oh!

russellpierce commented Dec 5, 2017

Uh oh!

wasperen commented Dec 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wasperen commented Dec 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ashb Dec 9, 2017

Choose a reason for hiding this comment

Uh oh!

wasperen Jan 13, 2018

Choose a reason for hiding this comment

Uh oh!

brokenjacobs commented Feb 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Fokko commented Feb 28, 2018

Uh oh!

gerardo commented Jun 26, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

wasperen commented Mar 30, 2017 •

edited

Loading

wasperen commented Apr 10, 2017 •

edited

Loading

codecov-io commented Apr 26, 2017 •

edited

Loading

wasperen commented Dec 7, 2017 •

edited

Loading

wasperen commented Dec 7, 2017 •

edited

Loading

brokenjacobs commented Feb 28, 2018 •

edited

Loading