[AIRFLOW-865] - Configure FTP connection mode by OmerJog · Pull Request #4535 · apache/airflow

OmerJog · 2019-01-15T12:45:28Z

Dear Airflow Maintainers,
This is a duplicate of stale PR #2069

Please accept this PR that addresses the following issues:

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

Testing Done:
Unit test

[AIRFLOW-865] - Configure FTP connection mode

#4526) [AIRFLOW-XXX] Include Los Angeles Times as a contributor in the Readme [AIRFLOW-3681] All GCP operators have now optional GCP Project ID (#4500)

…irflow-865

OmerJog · 2019-01-15T13:37:05Z

Test failures doesn't seem related to the PR.

zhongjiajie

CI test failed due to AttributeError

zhongjiajie · 2019-01-18T07:21:34Z

tests/contrib/hooks/test_ftp_hook.py

+
+        configuration.load_test_config()
+        db.merge_conn(
+            models.Connection(


The CI test show error like this

41) ERROR: test_ftp_active_mode (tests.contrib.hooks.test_ftp_hook.TestIntegrationFTPHook) ---------------------------------------------------------------------- Traceback (most recent call last): tests/contrib/hooks/test_ftp_hook.py line 140 in setUp models.Connection( AttributeError: module 'airflow.models' has no attribute 'Connection' ======================================================================

I think I failed due to models.Connection

@zhongjiajie You are correct. This happened since Connection was moved out of models.py in Master branch.
submitted a fix for this.

@OmerJog hope your PR can be merge

OmerJog · 2019-01-20T08:53:11Z

Can someone restart the error test? It wasn't finished.
@bolkedebruin you reviewed the previous PR. mind take a look at this?

zhongjiajie

@OmerJog you just commit some unnecessary change

tests/contrib/hooks/test_ftp_hook.py

Co-Authored-By: OmerJog <44576851+OmerJog@users.noreply.github.com>

…irflow-865

codecov-io · 2019-01-20T10:20:04Z

Codecov Report

Merging #4535 into master will increase coverage by 0.32%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #4535      +/-   ##
==========================================
+ Coverage   73.81%   74.14%   +0.32%     
==========================================
  Files         421      421              
  Lines       27662    27666       +4     
==========================================
+ Hits        20418    20512      +94     
+ Misses       7244     7154      -90

Impacted Files	Coverage Δ
airflow/contrib/hooks/ftp_hook.py	`69.56% <100%> (+9.2%)`	⬆️
airflow/hooks/dbapi_hook.py	`79.03% <0%> (+0.8%)`	⬆️
airflow/models/connection.py	`63.29% <0%> (+1.26%)`	⬆️
airflow/hooks/hive_hooks.py	`75.26% <0%> (+1.84%)`	⬆️
airflow/utils/sqlalchemy.py	`81.81% <0%> (+4.54%)`	⬆️
airflow/operators/mysql_operator.py	`100% <0%> (+100%)`	⬆️
airflow/operators/mysql_to_hive.py	`100% <0%> (+100%)`	⬆️

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 c0949cb...5e07c00. Read the comment docs.

codecov-io · 2019-01-20T10:20:04Z

Codecov Report

Merging #4535 into master will increase coverage by 0.32%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #4535      +/-   ##
==========================================
+ Coverage   73.81%   74.14%   +0.32%     
==========================================
  Files         421      421              
  Lines       27662    27666       +4     
==========================================
+ Hits        20418    20512      +94     
+ Misses       7244     7154      -90

Impacted Files	Coverage Δ
airflow/contrib/hooks/ftp_hook.py	`69.56% <100%> (+9.2%)`	⬆️
airflow/hooks/dbapi_hook.py	`79.03% <0%> (+0.8%)`	⬆️
airflow/models/connection.py	`63.29% <0%> (+1.26%)`	⬆️
airflow/hooks/hive_hooks.py	`75.26% <0%> (+1.84%)`	⬆️
airflow/utils/sqlalchemy.py	`81.81% <0%> (+4.54%)`	⬆️
airflow/operators/mysql_operator.py	`100% <0%> (+100%)`	⬆️
airflow/operators/mysql_to_hive.py	`100% <0%> (+100%)`	⬆️

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 c0949cb...5e07c00. Read the comment docs.

zhongjiajie · 2019-01-21T00:52:03Z

@Fokko PTAL

OmerJog · 2019-01-29T09:18:08Z

Any further comments?

Fokko

Looking good @OmerJog

Just checking if we don't change the default behaviour, but passive is on by default: https://docs.python.org/2/library/ftplib.html#ftplib.FTP.set_pasv

Merging to master.

OmerJog and others added 4 commits January 15, 2019 14:24

[AIRFLOW-865] - Configure FTP connection mode

5e9f6ac

[AIRFLOW-865] - Configure FTP connection mode

[AIRFLOW-865] - Configure FTP connection mode

6845b52

[AIRFLOW-865] - Configure FTP connection mode

[AIRFLOW-XXX] Include Los Angeles Times as a contributor in the Readme (

b97a4b7

#4526) [AIRFLOW-XXX] Include Los Angeles Times as a contributor in the Readme [AIRFLOW-3681] All GCP operators have now optional GCP Project ID (#4500)

Merge branch 'Airflow-865' of https://github.com/OmerJog/airflow into A…

8c3ecad

…irflow-865

zhongjiajie reviewed Jan 18, 2019

View reviewed changes

Fix errors

56481f3

zhongjiajie requested changes Jan 20, 2019

View reviewed changes

tests/contrib/hooks/test_ftp_hook.py Outdated Show resolved Hide resolved

zhongjiajie and others added 3 commits January 20, 2019 11:05

Update tests/contrib/hooks/test_ftp_hook.py

c7c084f

Co-Authored-By: OmerJog <44576851+OmerJog@users.noreply.github.com>

Update test_ftp_hook.py

b4948f6

Merge branch 'Airflow-865' of https://github.com/OmerJog/airflow into A…

5e07c00

…irflow-865

Fokko approved these changes Jan 29, 2019

View reviewed changes

Fokko merged commit 9f6e546 into apache:master Jan 29, 2019

ashb pushed a commit that referenced this pull request Apr 15, 2019

[AIRFLOW-865] Configure FTP connection mode (#4535)

aa93af4

wmorris75 pushed a commit to modmed-external/incubator-airflow that referenced this pull request Jul 29, 2019

[AIRFLOW-865] Configure FTP connection mode (apache#4535)

5619126

dharamsk pushed a commit to postmates/airflow that referenced this pull request Aug 8, 2019

[AIRFLOW-865] Configure FTP connection mode (apache#4535)

cfcc38a

Comments

Conversation

OmerJog commented Jan 15, 2019

Uh oh!

OmerJog commented Jan 15, 2019

Uh oh!

zhongjiajie left a comment

Choose a reason for hiding this comment

Uh oh!

zhongjiajie Jan 18, 2019

Choose a reason for hiding this comment

Uh oh!

OmerJog Jan 20, 2019

Choose a reason for hiding this comment

Uh oh!

zhongjiajie Jan 20, 2019

Choose a reason for hiding this comment

Uh oh!

OmerJog commented Jan 20, 2019

Uh oh!

zhongjiajie left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov-io commented Jan 20, 2019

Codecov Report

Uh oh!

codecov-io commented Jan 20, 2019

Codecov Report

Uh oh!

zhongjiajie commented Jan 21, 2019

Uh oh!

OmerJog commented Jan 29, 2019

Uh oh!

Fokko left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants