[AIRFLOW-6790] Add basic Tableau Integration by feluelle · Pull Request #7410 · apache/airflow

feluelle · 2020-02-13T10:45:57Z

This PR adds a TableauHook which wraps the tableauserverclient python library in an AirflowHook to connect to a Tableau Server. It also contains a TableauRefreshWorkbookOperator which refreshes a Tableau workbook / extract and a TableauJobStatusSensor which pokes the status of a Tableau Job until it has finished.

Issue link: AIRFLOW-6790

Make sure to mark the boxes below before creating PR: [x]

Description above provides context of the change
Commit message/PR title starts with [AIRFLOW-NNNN]. AIRFLOW-NNNN = JIRA ID^*
Unit tests coverage for changes (not needed for documentation changes)
Commits follow "How to write a good git commit message"
Relevant documentation is updated including usage instructions.
I will engage committers as explained in Contribution Workflow Example.

^* For document-only changes commit message can start with [AIRFLOW-XXXX].

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.
Read the Pull Request Guidelines for more information.

feluelle

Tableau does belong to Salesforce but can be used separately.

So the question is whether we want to install it separately or together with simple-salesforce.

WDYT?

airflow/providers/salesforce/hooks/tableau.py

airflow/providers/salesforce/operators/tableau_refresh_workbook.py

feluelle · 2020-02-17T14:13:58Z

Ready for another review @eladkal :)

codecov-io · 2020-02-18T18:12:35Z

Codecov Report

Merging #7410 into master will decrease coverage by 0.59%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master    #7410     +/-   ##
=========================================
- Coverage   86.66%   86.06%   -0.6%     
=========================================
  Files         877      881      +4     
  Lines       41252    41323     +71     
=========================================
- Hits        35751    35565    -186     
- Misses       5501     5758    +257

Impacted Files	Coverage Δ
airflow/models/connection.py	`95.07% <ø> (ø)`	⬆️
...s/salesforce/operators/tableau_refresh_workbook.py	`100% <100%> (ø)`
airflow/providers/salesforce/hooks/tableau.py	`100% <100%> (ø)`
...providers/salesforce/sensors/tableau_job_status.py	`100% <100%> (ø)`
...e/example_dags/example_tableau_refresh_workbook.py	`100% <100%> (ø)`
airflow/utils/db.py	`98.29% <100%> (+0.01%)`	⬆️
...flow/providers/apache/cassandra/hooks/cassandra.py	`21.51% <0%> (-72.16%)`	⬇️
...w/providers/apache/hive/operators/mysql_to_hive.py	`100% <0%> (ø)`	⬆️
airflow/kubernetes/volume_mount.py	`44.44% <0%> (-55.56%)`	⬇️
airflow/api/auth/backend/kerberos_auth.py	`83.09% <0%> (ø)`	⬆️
... and 20 more

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 7c95c81...f76a451. Read the comment docs.

Fokko

Nice work @feluelle. Having a Tableau operator would be a great addition. I've added some minor comments.

setup.py

Fokko · 2020-02-19T08:49:39Z

setup.py

I don't think Salesforce and Tableau are tightly integrated. I would split this into another group.

Okay. I am fine with this :)

If I put it into a separate group, does it mean it also belongs to the tableau provider? No, right?

@Fokko do you think we should move it to a new provider called Tableau?

Yes, that makes sense to me :)

airflow/providers/salesforce/sensors/tableau_job_status.py

airflow/providers/salesforce/operators/tableau_refresh_workbook.py

airflow/providers/salesforce/example_dags/example_tableau_refresh_workbook.py

eladkal

Tested against our Tableau server and it works as expected.

feluelle · 2020-02-20T14:12:32Z

@eladkal please take another look.

airflow/providers/salesforce/example_dags/example_tableau_refresh_workbook.py

airflow/providers/salesforce/hooks/tableau.py

- add TableauHook which wraps the tableauserverclient python library in an AirflowHook to connect to a Tableau Server - add TableauRefreshWorkbookOperator which refreshes a Tableau workbook / extract - add TableauJobStatusSensor which pokes the status of a Tableau Job until it has finished

feluelle · 2020-02-20T17:24:23Z

Ready for another round @Fokko @eladkal. I am going to continue tomorrow :)

Thank you for your time!

Fokko

Looking good @feluelle

eladkal · 2020-02-21T19:46:03Z

airflow/providers/salesforce/example_dags/example_tableau_refresh_workbook.py

+    # The following task queries the status of the workbook refresh job until it succeeds.
+    task_check_job_status = TableauJobStatusSensor(
+        site_id='my_site',
+        job_id="{{ ti.xcom_pull(task_ids='refresh_tableau_workbook') }}",


Shouldn't it be refresh_tableau_workbook_non_blocking ?

airflow/providers/salesforce/example_dags/example_tableau_refresh_workbook.py

eladkal · 2020-02-21T19:52:05Z

airflow/providers/salesforce/hooks/tableau.py

+    :type tableau_conn_id: str
+    """
+
+    def __init__(self, site_id: Optional[str] = None, tableau_conn_id: str = 'tableau_default'):


I asked in previous review about the switching the default of site_id from empty string to None. You had a good point that empty string is the default of the pacakge
https://tableau.github.io/server-client-python/docs/api-ref#authentication
Why did you enetualy decided to switch to None?

https://github.com/apache/airflow/pull/7410/files#diff-6332846625a8d68be6a4f2b74a47cc5dR56 takes care of that !

eladkal · 2020-02-21T19:57:27Z

tests/providers/salesforce/hooks/test_tableau.py

+
+        db.merge_conn(
+            models.Connection(
+                conn_id='tableau_test_password',


Why this connection is required? Can't it use the tableau_default that was added to utils/db.py?

airflow/providers/salesforce/example_dags/example_tableau_refresh_workbook.py

…esh_workbook.py

kaxil · 2020-02-22T01:11:10Z

Good work @feluelle 🎉

feluelle commented Feb 13, 2020

View reviewed changes

feluelle force-pushed the feature/AIRFLOW-6790-tableau-integration-basic branch from 7672250 to 7ee25f9 Compare February 13, 2020 10:55

boring-cyborg bot added the area:docs label Feb 13, 2020

eladkal reviewed Feb 13, 2020

View reviewed changes

feluelle force-pushed the feature/AIRFLOW-6790-tableau-integration-basic branch 2 times, most recently from 2a84b4f to f4ffb15 Compare February 17, 2020 14:12

feluelle force-pushed the feature/AIRFLOW-6790-tableau-integration-basic branch 3 times, most recently from 17e281b to f76a451 Compare February 18, 2020 17:27

feluelle requested review from Fokko, ashb, mik-laj and potiuk February 19, 2020 08:30

Fokko reviewed Feb 19, 2020

View reviewed changes

eladkal approved these changes Feb 20, 2020

View reviewed changes

feluelle force-pushed the feature/AIRFLOW-6790-tableau-integration-basic branch from 6bd1605 to 7b7082c Compare February 20, 2020 14:09

feluelle requested a review from Fokko February 20, 2020 14:11

feluelle commented Feb 20, 2020

View reviewed changes

airflow/providers/salesforce/example_dags/example_tableau_refresh_workbook.py Outdated Show resolved Hide resolved

feluelle changed the title ~~[AIRFLOW-6790] Add basic Tableau Integration~~ WIP: [AIRFLOW-6790] Add basic Tableau Integration Feb 20, 2020

feluelle changed the title ~~WIP: [AIRFLOW-6790] Add basic Tableau Integration~~ [WIP] [AIRFLOW-6790] Add basic Tableau Integration Feb 20, 2020

Fokko reviewed Feb 20, 2020

View reviewed changes

airflow/providers/salesforce/hooks/tableau.py Outdated Show resolved Hide resolved

feluelle force-pushed the feature/AIRFLOW-6790-tableau-integration-basic branch from 7b7082c to 8710e5a Compare February 20, 2020 16:04

feluelle changed the title ~~[WIP] [AIRFLOW-6790] Add basic Tableau Integration~~ [AIRFLOW-6790] Add basic Tableau Integration Feb 20, 2020

feluelle changed the title ~~[AIRFLOW-6790] Add basic Tableau Integration~~ [DO NOT MERGE] [AIRFLOW-6790] Add basic Tableau Integration Feb 20, 2020

feluelle force-pushed the feature/AIRFLOW-6790-tableau-integration-basic branch 2 times, most recently from f05e32a to 54c842c Compare February 20, 2020 16:57

feluelle changed the title ~~[DO NOT MERGE] [AIRFLOW-6790] Add basic Tableau Integration~~ [AIRFLOW-6790] Add basic Tableau Integration Feb 20, 2020

feluelle force-pushed the feature/AIRFLOW-6790-tableau-integration-basic branch from 54c842c to 7769a52 Compare February 20, 2020 17:23

Fokko approved these changes Feb 21, 2020

View reviewed changes

eladkal reviewed Feb 21, 2020

View reviewed changes

kaxil reviewed Feb 22, 2020

View reviewed changes

airflow/providers/salesforce/example_dags/example_tableau_refresh_workbook.py Outdated Show resolved Hide resolved

kaxil reviewed Feb 22, 2020

View reviewed changes

airflow/providers/salesforce/example_dags/example_tableau_refresh_workbook.py Outdated Show resolved Hide resolved

kaxil reviewed Feb 22, 2020

View reviewed changes

airflow/providers/salesforce/example_dags/example_tableau_refresh_workbook.py Outdated Show resolved Hide resolved

Apply suggestions from code review

87b8186

kaxil reviewed Feb 22, 2020

View reviewed changes

airflow/providers/salesforce/example_dags/example_tableau_refresh_workbook.py Outdated Show resolved Hide resolved

Update airflow/providers/salesforce/example_dags/example_tableau_refr…

479d29c

…esh_workbook.py

kaxil approved these changes Feb 22, 2020

View reviewed changes

kaxil merged commit a9ad0a9 into apache:master Feb 22, 2020

galuszkak pushed a commit to FlyrInc/apache-airflow that referenced this pull request Mar 5, 2020

[AIRFLOW-6790] Add basic Tableau Integration (apache#7410)

84270ca

feluelle mentioned this pull request Mar 21, 2020

[AIRFLOW-6944] Allow AWS DataSync to "catch up" when Task is already … #7585

Closed

6 tasks

feluelle deleted the feature/AIRFLOW-6790-tableau-integration-basic branch April 30, 2020 07:08

Conversation

feluelle commented Feb 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

feluelle left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

feluelle commented Feb 17, 2020

Uh oh!

codecov-io commented Feb 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Fokko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eladkal left a comment

Choose a reason for hiding this comment

Uh oh!

feluelle commented Feb 20, 2020

Uh oh!

Uh oh!

Uh oh!

feluelle commented Feb 20, 2020

Uh oh!

Fokko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kaxil commented Feb 22, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

feluelle commented Feb 13, 2020 •

edited

Loading

feluelle left a comment •

edited

Loading

codecov-io commented Feb 18, 2020 •

edited

Loading