-
Notifications
You must be signed in to change notification settings - Fork 34
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
default to standard SSL port for HTTPS URIs #105
Comments
Originally this was done to force people to be explicit about wanting ssl. With the advent of gabbi-run, the way it does targeting, and various other refactorings, clearly something has become confused and lost since then. So yeah, there's definitely room for improvement here. |
This has some interesting issues. It is easy enough to get the port set to 443 at the runner level, however, when we get around to actually doing the request, since Because httplib2 has this code: (scheme, authority, request_uri, defrag_uri) = urlnorm(uri)
domain_port = authority.split(":")[0:2]
if len(domain_port) == 2 and domain_port[1] == '443' and scheme == 'http':
scheme = 'https'
authority = domain_port[0] everything works. But this is an implementation detail that we should not rely on. So I'm thinking the underlying error/issue is the way in which SSL is being managed (some of which is discussed in #50). Changing that would be a fairly fundamental thing, unless we do something like manage the test base somehow. Ideas? |
As agreed on IRC, the solution will likely be to set |
Based on the existence of #138 I've looked into this a bit more and I think what can be done is in the small number of entry points to test_suite_from_dict we can directly manipulate the value of defaults in the dict that is passed. More details to come shortly, but it is likely this will be slightly tidier if we merge the runner-cleanup branch first. |
build_tests gains a require_ssl argument which, if set to True, makes all the loaded test suites default to 'ssl: True'. gabbi-run will interpret a target containing 'https' as meaning that the tests in the provided yaml should default to 'ssl: True'. Fixes: #50 Fixes: #105 Fixes: #138 The changes here are the naive basics to get the desired behavior. There's an existing cleanup branch on which we can clean this up later.
results in "[Errno 61] Connection refused", whereas
works succeeds.
It seems appropriate to default to port 443 for HTTPS URIs.
The text was updated successfully, but these errors were encountered: