Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix(api) parsing service url default to 443 for https #3358

Merged
merged 1 commit into from
Apr 9, 2018
Merged

fix(api) parsing service url default to 443 for https #3358

merged 1 commit into from
Apr 9, 2018

Conversation

Tieske
Copy link
Member

@Tieske Tieske commented Mar 29, 2018

The url convenience property was parsed, but defaulted
to port 80, even for 'https' schemes. Updated to 443.

fixes #3357

@hishamhm
Copy link
Contributor

@Tieske Travis failures are related: url.parse may not return a scheme in invalid URLs.

Also, no need to do :lower() and in fact I'd prefer if it didn't: the schema validator won't allow uppercase schemes anyway (the lenience there makes the code misleading, as it makes it seem that uppercase inputs are accepted).

@thibaultcha thibaultcha added the pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. label Mar 29, 2018
@bungle
Copy link
Member

bungle commented Mar 29, 2018

I also agree that lower can be removed. And we need to add test or too., as usual

@Tieske Tieske force-pushed the fix/service-url branch 2 times, most recently from cad6215 to 5ec123d Compare April 2, 2018 07:14
@Tieske Tieske added pr/please review and removed pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. labels Apr 2, 2018
@Tieske
Copy link
Member Author

Tieske commented Apr 2, 2018

@bungle @hishamhm comments addressed

bungle
bungle reviewed Apr 2, 2018
View reviewed changes
Copy link
Member

@bungle bungle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, though now that @hishamhm mentioned about unneccessary lower, I was thinking. is that tonumber unneccessary as well?

thibaultcha
thibaultcha previously requested changes Apr 2, 2018
View reviewed changes
Copy link
Member

@thibaultcha thibaultcha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now, it seems like there still is a related failure in Travis CI

spec/02-integration/04-admin_api/21-services_routes_spec.lua Outdated
@@ -97,6 +97,31 @@ for _, strategy in helpers.each_strategy() do
assert.equals(60000, json.read_timeout)
end
end)

it_content_types("creates a service with url (https based)", function(content_type)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't the proper name for this test: 'port' defaults to 443 when 'url' scheme is https?

@Tieske
fix(api) parsing service url default to 443 for https
90bef7d 
The `url` convenience property was parsed, but defaulted
to port 80, even for 'https' schemes. Updated to 443.

fixes #3357
@Tieske Tieske force-pushed the fix/service-url branch from 5ec123d to 90bef7d Compare April 3, 2018 09:02
@Tieske
Copy link
Member Author

Tieske commented Apr 3, 2018

description updated, test fixed, and 'tonumber' left alone since it is required.

@bungle bungle mentioned this pull request Apr 3, 2018
Closed
@bungle bungle added pr/ready This PR is considered ready and can be merged at anytime (given it received no subsequent changes) and removed pr/please review labels Apr 9, 2018
@bungle bungle dismissed thibaultcha’s stale review April 9, 2018 09:54

the reported error in test suite is fixed

@thibaultcha thibaultcha merged commit 11b1694 into master Apr 9, 2018
@thibaultcha thibaultcha deleted the fix/service-url branch April 9, 2018 18:26
kikito pushed a commit to kikito/kong that referenced this pull request Apr 10, 2018
@Tieske @kikito
fix(admin) set port as 443 when Service 'url' uses 'https'
37fd8d2 
The `url` convenience argument was parsed, but defaulted to
port 80 even for `https` schemes.

Fix Kong#3357 
From Kong#3358
@thibaultcha thibaultcha mentioned this pull request May 8, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thibaultcha thibaultcha thibaultcha approved these changes

@bungle bungle bungle approved these changes

Assignees
No one assigned
Labels
pr/ready This PR is considered ready and can be merged at anytime (given it received no subsequent changes)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

service 'url' creation defaults to port 80 for https
4 participants
@Tieske @hishamhm @bungle @thibaultcha