You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2020/03/10 16:14:34 UTC
[GitHub] [airflow] NBardelot opened a new pull request #7682: [AIRFLOW-7031]
Airflow WinRM endpoint is hardcoded to HTTP
NBardelot opened a new pull request #7682: [AIRFLOW-7031] Airflow WinRM endpoint is hardcoded to HTTP
URL: https://github.com/apache/airflow/pull/7682
This commit fixes the issue and avoid errors like this:
```
ERROR - Error connecting to host: 1.2.3.4, error: HTTPConnectionPool(host='1.2.3.4', port=5986): Read timed out. (read timeout=30)
```
Even when `service` is set to `HTTPS` because the WinRM hook `endpoint`'s default configuration was hardcoded with the `http://` protocol.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [airflow] NBardelot commented on a change in pull request #7682:
[AIRFLOW-7031] Airflow WinRM endpoint is hardcoded to HTTP
Posted by GitBox <gi...@apache.org>.
NBardelot commented on a change in pull request #7682: [AIRFLOW-7031] Airflow WinRM endpoint is hardcoded to HTTP
URL: https://github.com/apache/airflow/pull/7682#discussion_r397014266
##########
File path: airflow/providers/microsoft/winrm/hooks/winrm.py
##########
@@ -199,7 +199,7 @@ def get_conn(self):
# If endpoint is not set, then build a standard wsman endpoint from host and port.
if not self.endpoint:
- self.endpoint = 'http://{0}:{1}/wsman'.format(self.remote_host, self.remote_port)
+ self.endpoint = '{0}://{1}:{2}/wsman'.format(self.service.lower(), self.remote_host, self.remote_port)
Review comment:
I 100% understand what you mean, but I believe it's best to use lowercase. The old behavious (pre-commit) was `http` lowercase. My goal is to keep the code behaving like it did before by default, to avoid regression (uppercase `HTTP` while RFC-compliant *is* a change in behaviour, thus it could have unexpected side-effects for people using the hook).
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on a change in pull request #7682:
[AIRFLOW-7031] Airflow WinRM endpoint is hardcoded to HTTP
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #7682: [AIRFLOW-7031] Airflow WinRM endpoint is hardcoded to HTTP
URL: https://github.com/apache/airflow/pull/7682#discussion_r397128607
##########
File path: airflow/providers/microsoft/winrm/hooks/winrm.py
##########
@@ -199,7 +199,7 @@ def get_conn(self):
# If endpoint is not set, then build a standard wsman endpoint from host and port.
if not self.endpoint:
- self.endpoint = 'http://{0}:{1}/wsman'.format(self.remote_host, self.remote_port)
+ self.endpoint = '{0}://{1}:{2}/wsman'.format(self.service.lower(), self.remote_host, self.remote_port)
Review comment:
That would be super-valuable contribution!
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [airflow] ad-m commented on a change in pull request #7682:
[AIRFLOW-7031] Airflow WinRM endpoint is hardcoded to HTTP
Posted by GitBox <gi...@apache.org>.
ad-m commented on a change in pull request #7682: [AIRFLOW-7031] Airflow WinRM endpoint is hardcoded to HTTP
URL: https://github.com/apache/airflow/pull/7682#discussion_r392539986
##########
File path: airflow/providers/microsoft/winrm/hooks/winrm.py
##########
@@ -199,7 +199,7 @@ def get_conn(self):
# If endpoint is not set, then build a standard wsman endpoint from host and port.
if not self.endpoint:
- self.endpoint = 'http://{0}:{1}/wsman'.format(self.remote_host, self.remote_port)
+ self.endpoint = '{0}://{1}:{2}/wsman'.format(self.service.lower(), self.remote_host, self.remote_port)
Review comment:
```suggestion
self.endpoint = '{0}://{1}:{2}/wsman'.format(self.service, self.remote_host, self.remote_port)
```
Schema section of URL are case-insensitive and implementation should accept uppercase letters as equivalent to lowercase in scheme names (e.g., allow "HTTP" as well as "http"). See RFC 3986 for details.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [airflow] ad-m commented on a change in pull request #7682:
[AIRFLOW-7031] Airflow WinRM endpoint is hardcoded to HTTP
Posted by GitBox <gi...@apache.org>.
ad-m commented on a change in pull request #7682: [AIRFLOW-7031] Airflow WinRM endpoint is hardcoded to HTTP
URL: https://github.com/apache/airflow/pull/7682#discussion_r397102749
##########
File path: airflow/providers/microsoft/winrm/hooks/winrm.py
##########
@@ -199,7 +199,7 @@ def get_conn(self):
# If endpoint is not set, then build a standard wsman endpoint from host and port.
if not self.endpoint:
- self.endpoint = 'http://{0}:{1}/wsman'.format(self.remote_host, self.remote_port)
+ self.endpoint = '{0}://{1}:{2}/wsman'.format(self.service.lower(), self.remote_host, self.remote_port)
Review comment:
@turbaszek , what is the recommended policy of the change management of Airflow project in this respect to use value in accordance with RFC without undue transformation? Should we change the default value to use "http" (now "HTTP" in the constructor)? Then transformation should not be necessary then.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on a change in pull request #7682:
[AIRFLOW-7031] Airflow WinRM endpoint is hardcoded to HTTP
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #7682: [AIRFLOW-7031] Airflow WinRM endpoint is hardcoded to HTTP
URL: https://github.com/apache/airflow/pull/7682#discussion_r397128015
##########
File path: airflow/providers/microsoft/winrm/hooks/winrm.py
##########
@@ -199,7 +199,7 @@ def get_conn(self):
# If endpoint is not set, then build a standard wsman endpoint from host and port.
if not self.endpoint:
- self.endpoint = 'http://{0}:{1}/wsman'.format(self.remote_host, self.remote_port)
+ self.endpoint = '{0}://{1}:{2}/wsman'.format(self.service.lower(), self.remote_host, self.remote_port)
Review comment:
Hey @ad-m -> we have no policy for that. But If you would like to propose some - I invite you to start discussion at the devlist. This is community-driven project and You seem to be knowledgeable in this area, I would be very happy if someone like you could propose and discuss with the rest of the community any proposals you might have.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [airflow] NBardelot commented on a change in pull request #7682:
[AIRFLOW-7031] Airflow WinRM endpoint is hardcoded to HTTP
Posted by GitBox <gi...@apache.org>.
NBardelot commented on a change in pull request #7682: [AIRFLOW-7031] Airflow WinRM endpoint is hardcoded to HTTP
URL: https://github.com/apache/airflow/pull/7682#discussion_r397014266
##########
File path: airflow/providers/microsoft/winrm/hooks/winrm.py
##########
@@ -199,7 +199,7 @@ def get_conn(self):
# If endpoint is not set, then build a standard wsman endpoint from host and port.
if not self.endpoint:
- self.endpoint = 'http://{0}:{1}/wsman'.format(self.remote_host, self.remote_port)
+ self.endpoint = '{0}://{1}:{2}/wsman'.format(self.service.lower(), self.remote_host, self.remote_port)
Review comment:
I 100% understand what you mean, but I believe it's best to use lowercase. The old behavious (pre-commit) was `http` lowercase. My goal is to keep the code behaving like it did before by default, to avoid regression (uppercase `HTTP` while RFC-compliant *is* a change in behaviour, thus it could have unexpected side-effects).
----------------------------------------------------------------
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
With regards,
Apache Git Services