[GitHub] [airflow] feluelle commented on a change in pull request #6576: [AIRFLOW-5922] Add option to specify the mysql client library used in MySqlHook

2020-03-06 Thread GitBox 
feluelle commented on a change in pull request #6576: [AIRFLOW-5922] Add option 
to specify the mysql client library used in MySqlHook
URL: https://github.com/apache/airflow/pull/6576#discussion_r389087612
 
 

 ##
 File path: airflow/providers/mysql/hooks/mysql.py
 ##
 @@ -113,8 +107,44 @@ def get_conn(self):
 conn_config['unix_socket'] = conn.extra_dejson['unix_socket']
 if local_infile:
 conn_config["local_infile"] = 1
-conn = MySQLdb.connect(**conn_config)
-return conn
+return conn_config
+
+def _get_conn_config_mysql_connector_python(self, conn):
+conn_config = {
+'user': conn.login,
+'password': conn.password or '',
+'host': conn.host or 'localhost',
+'database': self.schema or conn.schema or '',
+'port': int(conn.port) if conn.port else 3306
+}
+
+if conn.extra_dejson.get('allow_local_infile', False):
+conn_config["allow_local_infile"] = True
+
+return conn_config
+
+def get_conn(self):
+"""
+Establishes a connection to a mysql database
+by extracting the connection configuration from the Airflow connection.
+
+.. note:: By default it connects to the database via the mysqlclient 
library.
+But you can also choose the mysql-connector-python library which 
lets you connect through ssl
+without any further ssl parameters required.
+
+:return: a mysql connection object
+"""
+conn = self.connection or self.get_connection(self.mysql_conn_id)  # 
pylint: disable=no-member
+
+client_name = conn.extra_dejson.get('client', 'mysqlclient')
 
 Review comment:
   Please take a look at my latest changes :)


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] feluelle commented on a change in pull request #6576: [AIRFLOW-5922] Add option to specify the mysql client library used in MySqlHook

2020-03-04 Thread GitBox 
feluelle commented on a change in pull request #6576: [AIRFLOW-5922] Add option 
to specify the mysql client library used in MySqlHook
URL: https://github.com/apache/airflow/pull/6576#discussion_r387850309
 
 

 ##
 File path: setup.py
 ##
 @@ -286,6 +286,7 @@ def write_version(filename: str = 
os.path.join(*[dirname(__file__), "airflow", "
 'pymssql~=2.1.1',
 ]
 mysql = [
+'mysql-connector-python==8.0.18',
 
 Review comment:
   ```suggestion
   'mysql-connector-python>=8.0.11,mysql-connector-python<=8.0.18',
   ```
   
   According to 
https://dev.mysql.com/doc/relnotes/connector-python/en/news-8-0.html 8.0.11 was 
the first release. WDYT?


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] feluelle commented on a change in pull request #6576: [AIRFLOW-5922] Add option to specify the mysql client library used in MySqlHook

2020-03-04 Thread GitBox 
feluelle commented on a change in pull request #6576: [AIRFLOW-5922] Add option 
to specify the mysql client library used in MySqlHook
URL: https://github.com/apache/airflow/pull/6576#discussion_r387850309
 
 

 ##
 File path: setup.py
 ##
 @@ -286,6 +286,7 @@ def write_version(filename: str = 
os.path.join(*[dirname(__file__), "airflow", "
 'pymssql~=2.1.1',
 ]
 mysql = [
+'mysql-connector-python==8.0.18',
 
 Review comment:
   ```suggestion
   'mysql-connector-python>=8.0.11,mysql-connector-python<8.0.19',
   ```
   
   According to 
https://dev.mysql.com/doc/relnotes/connector-python/en/news-8-0.html 8.0.11 was 
the first release. WDYT?


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] feluelle commented on a change in pull request #6576: [AIRFLOW-5922] Add option to specify the mysql client library used in MySqlHook

2020-03-04 Thread GitBox 
feluelle commented on a change in pull request #6576: [AIRFLOW-5922] Add option 
to specify the mysql client library used in MySqlHook
URL: https://github.com/apache/airflow/pull/6576#discussion_r387850309
 
 

 ##
 File path: setup.py
 ##
 @@ -286,6 +286,7 @@ def write_version(filename: str = 
os.path.join(*[dirname(__file__), "airflow", "
 'pymssql~=2.1.1',
 ]
 mysql = [
+'mysql-connector-python==8.0.18',
 
 Review comment:
   ```suggestion
   'mysql-connector-python>=8.0.0,mysql-connector-python<8.0.19',
   ```
   
   WDYT?


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] feluelle commented on a change in pull request #6576: [AIRFLOW-5922] Add option to specify the mysql client library used in MySqlHook

2020-03-04 Thread GitBox 
feluelle commented on a change in pull request #6576: [AIRFLOW-5922] Add option 
to specify the mysql client library used in MySqlHook
URL: https://github.com/apache/airflow/pull/6576#discussion_r387850309
 
 

 ##
 File path: setup.py
 ##
 @@ -286,6 +286,7 @@ def write_version(filename: str = 
os.path.join(*[dirname(__file__), "airflow", "
 'pymssql~=2.1.1',
 ]
 mysql = [
+'mysql-connector-python==8.0.18',
 
 Review comment:
   ```suggestion
   'mysql-connector-python>=8.0.5,mysql-connector-python<8.0.19',
   ```
   
   WDYT?


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] feluelle commented on a change in pull request #6576: [AIRFLOW-5922] Add option to specify the mysql client library used in MySqlHook

2020-03-04 Thread GitBox 
feluelle commented on a change in pull request #6576: [AIRFLOW-5922] Add option 
to specify the mysql client library used in MySqlHook
URL: https://github.com/apache/airflow/pull/6576#discussion_r387850309
 
 

 ##
 File path: setup.py
 ##
 @@ -286,6 +286,7 @@ def write_version(filename: str = 
os.path.join(*[dirname(__file__), "airflow", "
 'pymssql~=2.1.1',
 ]
 mysql = [
+'mysql-connector-python==8.0.18',
 
 Review comment:
   ```suggestion
   'mysql-connector-python>=8.0.0,mysql-connector-python<8.0.19',
   ```


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] feluelle commented on a change in pull request #6576: [AIRFLOW-5922] Add option to specify the mysql client library used in MySqlHook

2020-03-04 Thread GitBox 
feluelle commented on a change in pull request #6576: [AIRFLOW-5922] Add option 
to specify the mysql client library used in MySqlHook
URL: https://github.com/apache/airflow/pull/6576#discussion_r387849537
 
 

 ##
 File path: setup.py
 ##
 @@ -286,6 +286,7 @@ def write_version(filename: str = 
os.path.join(*[dirname(__file__), "airflow", "
 'pymssql~=2.1.1',
 ]
 mysql = [
+'mysql-connector-python==8.0.18',
 
 Review comment:
   8.0.19 does not work: 
https://github.com/apache/airflow/pull/6576#issuecomment-591519540


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] feluelle commented on a change in pull request #6576: [AIRFLOW-5922] Add option to specify the mysql client library used in MySqlHook

2020-03-04 Thread GitBox 
feluelle commented on a change in pull request #6576: [AIRFLOW-5922] Add option 
to specify the mysql client library used in MySqlHook
URL: https://github.com/apache/airflow/pull/6576#discussion_r387691301
 
 

 ##
 File path: airflow/providers/mysql/hooks/mysql.py
 ##
 @@ -16,10 +16,13 @@
 # specific language governing permissions and limitations
 # under the License.
 
+"""
+This module allows to connect to a MySQL database.
+"""
+
 import json
 
 import MySQLdb
 
 Review comment:
   Not necessarily. I can move it to the `mysqlclient` code (so it will only be 
imported when used).


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] feluelle commented on a change in pull request #6576: [AIRFLOW-5922] Add option to specify the mysql client library used in MySqlHook

2020-03-04 Thread GitBox 
feluelle commented on a change in pull request #6576: [AIRFLOW-5922] Add option 
to specify the mysql client library used in MySqlHook
URL: https://github.com/apache/airflow/pull/6576#discussion_r387690189
 
 

 ##
 File path: airflow/providers/mysql/hooks/mysql.py
 ##
 @@ -113,8 +107,44 @@ def get_conn(self):
 conn_config['unix_socket'] = conn.extra_dejson['unix_socket']
 if local_infile:
 conn_config["local_infile"] = 1
-conn = MySQLdb.connect(**conn_config)
-return conn
+return conn_config
+
+def _get_conn_config_mysql_connector_python(self, conn):
+conn_config = {
+'user': conn.login,
+'password': conn.password or '',
+'host': conn.host or 'localhost',
+'database': self.schema or conn.schema or '',
+'port': int(conn.port) if conn.port else 3306
+}
+
+if conn.extra_dejson.get('allow_local_infile', False):
+conn_config["allow_local_infile"] = True
+
+return conn_config
+
+def get_conn(self):
+"""
+Establishes a connection to a mysql database
+by extracting the connection configuration from the Airflow connection.
+
+.. note:: By default it connects to the database via the mysqlclient 
library.
+But you can also choose the mysql-connector-python library which 
lets you connect through ssl
+without any further ssl parameters required.
+
+:return: a mysql connection object
+"""
+conn = self.connection or self.get_connection(self.mysql_conn_id)  # 
pylint: disable=no-member
+
+client_name = conn.extra_dejson.get('client', 'mysqlclient')
+
+if client_name == 'mysql-connector-python':
+import mysql.connector
+conn_config = self._get_conn_config_mysql_connector_python(conn)
+return mysql.connector.connect(**conn_config)
+
+conn_config = self._get_conn_config_mysql_client(conn)
 
 Review comment:
   Like this?
   
   ```python
   client_name = conn.extra_dejson.get('client', 'mysqlclient')
   
   if client_name == 'mysqlclient':
   ...
   return ...
   
   if client_name == 'mysql-connector-python':
   ...
   return ...
   
   raise ValueError('Unknown MySQL client name provided!')
   ```


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] feluelle commented on a change in pull request #6576: [AIRFLOW-5922] Add option to specify the mysql client library used in MySqlHook

2020-03-04 Thread GitBox 
feluelle commented on a change in pull request #6576: [AIRFLOW-5922] Add option 
to specify the mysql client library used in MySqlHook
URL: https://github.com/apache/airflow/pull/6576#discussion_r387690189
 
 

 ##
 File path: airflow/providers/mysql/hooks/mysql.py
 ##
 @@ -113,8 +107,44 @@ def get_conn(self):
 conn_config['unix_socket'] = conn.extra_dejson['unix_socket']
 if local_infile:
 conn_config["local_infile"] = 1
-conn = MySQLdb.connect(**conn_config)
-return conn
+return conn_config
+
+def _get_conn_config_mysql_connector_python(self, conn):
+conn_config = {
+'user': conn.login,
+'password': conn.password or '',
+'host': conn.host or 'localhost',
+'database': self.schema or conn.schema or '',
+'port': int(conn.port) if conn.port else 3306
+}
+
+if conn.extra_dejson.get('allow_local_infile', False):
+conn_config["allow_local_infile"] = True
+
+return conn_config
+
+def get_conn(self):
+"""
+Establishes a connection to a mysql database
+by extracting the connection configuration from the Airflow connection.
+
+.. note:: By default it connects to the database via the mysqlclient 
library.
+But you can also choose the mysql-connector-python library which 
lets you connect through ssl
+without any further ssl parameters required.
+
+:return: a mysql connection object
+"""
+conn = self.connection or self.get_connection(self.mysql_conn_id)  # 
pylint: disable=no-member
+
+client_name = conn.extra_dejson.get('client', 'mysqlclient')
+
+if client_name == 'mysql-connector-python':
+import mysql.connector
+conn_config = self._get_conn_config_mysql_connector_python(conn)
+return mysql.connector.connect(**conn_config)
+
+conn_config = self._get_conn_config_mysql_client(conn)
 
 Review comment:
   Like this?
   
   ```python
   client_name = conn.extra_dejson.get('client', 'mysqlclient')
   
   if client_name == 'mysqlclient':
   ...
   
   if client_name == 'mysql-connector-python':
   ...
   
   raise ValueError('Unknown MySQL client name provided!')
   ```


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] feluelle commented on a change in pull request #6576: [AIRFLOW-5922] Add option to specify the mysql client library used in MySqlHook

2020-02-27 Thread GitBox 
feluelle commented on a change in pull request #6576: [AIRFLOW-5922] Add option 
to specify the mysql client library used in MySqlHook
URL: https://github.com/apache/airflow/pull/6576#discussion_r385217048
 
 

 ##
 File path: airflow/providers/mysql/hooks/mysql.py
 ##
 @@ -113,8 +107,44 @@ def get_conn(self):
 conn_config['unix_socket'] = conn.extra_dejson['unix_socket']
 if local_infile:
 conn_config["local_infile"] = 1
-conn = MySQLdb.connect(**conn_config)
-return conn
+return conn_config
+
+def _get_conn_config_mysql_connector_python(self, conn):
+conn_config = {
+'user': conn.login,
+'password': conn.password or '',
+'host': conn.host or 'localhost',
+'database': self.schema or conn.schema or '',
+'port': int(conn.port) if conn.port else 3306
+}
+
+if conn.extra_dejson.get('allow_local_infile', False):
+conn_config["allow_local_infile"] = True
+
+return conn_config
+
+def get_conn(self):
+"""
+Establishes a connection to a mysql database
+by extracting the connection configuration from the Airflow connection.
+
+.. note:: By default it connects to the database via the mysqlclient 
library.
+But you can also choose the mysql-connector-python library which 
lets you connect through ssl
+without any further ssl parameters required.
+
+:return: a mysql connection object
+"""
+conn = self.connection or self.get_connection(self.mysql_conn_id)  # 
pylint: disable=no-member
 
 Review comment:
   https://github.com/apache/airflow/pull/6576#discussion_r362945137 :D 


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] feluelle commented on a change in pull request #6576: [AIRFLOW-5922] Add option to specify the mysql client library used in MySqlHook

2020-01-03 Thread GitBox 
feluelle commented on a change in pull request #6576: [AIRFLOW-5922] Add option 
to specify the mysql client library used in MySqlHook
URL: https://github.com/apache/airflow/pull/6576#discussion_r362945137
 
 

 ##
 File path: airflow/hooks/mysql_hook.py
 ##
 @@ -114,8 +111,46 @@ def get_conn(self):
 conn_config['unix_socket'] = conn.extra_dejson['unix_socket']
 if local_infile:
 conn_config["local_infile"] = 1
-conn = MySQLdb.connect(**conn_config)
-return conn
+return conn_config
+
+def _get_conn_config_mysql_connector_python(self, conn):
+conn_config = {
+'user': conn.login,
+'password': conn.password or '',
+'host': conn.host or 'localhost',
+'database': self.schema or conn.schema or ''
+}
+
+if not conn.port:
+conn_config['port'] = 3306
+else:
+conn_config['port'] = int(conn.port)
+
+if conn.extra_dejson.get('allow_local_infile', False):
+conn_config["allow_local_infile"] = True
+
+return conn_config
+
+def get_conn(self):
+"""
+Establishes a connection to a mysql database
+by extracting the connection configuration from the Airflow connection.
+
+.. note:: By default it connects to the database via the mysqlclient 
library.
+But you can also choose the mysql-connector-python library which 
lets you connect through ssl
+without any further ssl parameters required.
+
+:return: a mysql connection object
+"""
+conn = self.connection or self.get_connection(self.mysql_conn_id)  # 
pylint: disable=no-member
 
 Review comment:
   Because `mysql_conn_id` is only accessible because of `conn_name_attr = 
'mysql_conn_id'`


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:
us...@infra.apache.org


With regards,
Apache Git Services