[GitHub] [airflow] ciancolo commented on a change in pull request #16365: Implemented Tableau Hook connection through SSL

[GitBox <gi...@apache.org>]

ciancolo commented on a change in pull request #16365:
URL: https://github.com/apache/airflow/pull/16365#discussion_r649751086



##########
File path: airflow/providers/tableau/hooks/tableau.py
##########
@@ -61,7 +61,9 @@ def __init__(self, site_id: Optional[str] = None, tableau_conn_id: str = default
         self.tableau_conn_id = tableau_conn_id
         self.conn = self.get_connection(self.tableau_conn_id)
         self.site_id = site_id or self.conn.extra_dejson.get('site_id', '')
-        self.server = Server(self.conn.host, use_server_version=True)
+        self.server = Server(self.conn.host)
+        self.server.add_http_options(options_dict={'verify': self.conn.extra_dejson.get('verify', False)})

Review comment:
       Thank you for the review. 
   I completely agree with the changes you are requiring; in the next few days, I will implement them. 
   
   For the default value of `verify` I set the value False following the same Tableau example you linked above. The Tableau doc is not clear and I thought it was the right value in case of an HTTP connection.
   However, looking deeper into the implementation code of `tableauserverclient` module ([here](https://github.com/tableau/server-client-python/blob/ce37a2063eddf317aaf2703aa3ebccf8053e1c8a/tableauserverclient/server/server.py)) I think it is ok (or better) also set `verify=True` as default. I mean with the current Tableau Hook implementation it works with HTTP connection and with that default we can avoid not verify SSL connection.
   So I agree with you to set `verify=True` as default.
   




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