New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix secure propagation to connection class #972

Merged
merged 4 commits into from Jan 10, 2017

Conversation

Projects
None yet
3 participants
@tonybaloney
Contributor

tonybaloney commented Jan 10, 2017

Since there is no longer a secure/insecure connection class, they are one and the same.

This change adds new tests to check that

  • When a connection is defined as secure, the connection class will use the https scheme.
  • When a connection scheme implies a port, the port is not included in the URL
  • When a connection defines secure and an unusual port, the scheme is propagated

It uncovered 1 issue with secure connections not being explicit for unusual ports.

tonybaloney added some commits Jan 10, 2017

@tonybaloney

This comment has been minimized.

Show comment
Hide comment
@tonybaloney

tonybaloney Jan 10, 2017

Contributor

@Kami this is the second change to fix the issue you mentioned.

Contributor

tonybaloney commented Jan 10, 2017

@Kami this is the second change to fix the issue you mentioned.

@tonybaloney

This comment has been minimized.

Show comment
Hide comment
@tonybaloney

tonybaloney Jan 10, 2017

Contributor

merging

Contributor

tonybaloney commented Jan 10, 2017

merging

@asfgit asfgit merged commit e5d2cfd into apache:trunk Jan 10, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

asfgit pushed a commit that referenced this pull request Jan 10, 2017

@@ -162,9 +162,10 @@ class LibcloudConnection(LibcloudBaseConnection):
host = None
response = None
def __init__(self, host, port, **kwargs):
def __init__(self, host, port, secure=None, **kwargs):
scheme = 'https' if secure is not None and secure else 'http'

This comment has been minimized.

@allardhoeve

allardhoeve Jan 10, 2017

Contributor

I think the defaults would be more clear if this read:

def __init__(self, host, port, secure=None, **kwargs):
    if secure is None and port == 443:
        secure = True
    scheme = 'https' if secure else 'http'

    # ... rest of the code

I dislike inline logic because it is hard to find for the casual reader.

Also, a docstring would be nice.

@allardhoeve

allardhoeve Jan 10, 2017

Contributor

I think the defaults would be more clear if this read:

def __init__(self, host, port, secure=None, **kwargs):
    if secure is None and port == 443:
        secure = True
    scheme = 'https' if secure else 'http'

    # ... rest of the code

I dislike inline logic because it is hard to find for the casual reader.

Also, a docstring would be nice.

This comment has been minimized.

@tonybaloney

tonybaloney Jan 10, 2017

Contributor

👍

@tonybaloney

tonybaloney Jan 10, 2017

Contributor

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment