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.

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

Strip protocol part, present e.g. if in KDE + manual proxy mode. #5195

Merged
merged 1 commit into from
Jan 5, 2023

Conversation

sdedic
Copy link
Member

@sdedic sdedic commented Jan 4, 2023

I've noticed bad behaviour in KDE when the system proxy is specified manually. The kioslaverc then contains settings like

ftpProxy=ftp://proxy.acme.com:80
httpProxy=http://proxy.acme.com:80
httpsProxy=http://proxy.acme.com:80
socksProxy=socks://proxy.acme.com:80

and the proxy string then contains not only port specification, but the protocol part as well. The error can be seen in Tools | Options, when System proxy is selected (and KDE has a manual system proxy) - the "Test connection" button action fails with hostname prefixed by http://.

This PR strips the PR part from the host string.

@sdedic sdedic added kind:bug Bug report or fix Platform [ci] enable platform tests (platform/*) labels Jan 4, 2023
@sdedic sdedic added this to the NB17 milestone Jan 4, 2023
@sdedic sdedic self-assigned this Jan 4, 2023
Copy link
Contributor

@matthiasblaesing matthiasblaesing left a comment

Choose a reason for hiding this comment

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

Seems reasonable. The settings format seems a bit strange and this might be better in the KDE only part, but I can imagine people using that format in the http_proxy environment format. There might be an issue if people call their hosts like that, but then they will be in pain anyway.

Oh and the unittest failure in CSS look weird, I'll restart the test.

@matthiasblaesing
Copy link
Contributor

Was a bit strange, restarting just the failing job moved the problem from CSS editor to option editor. Rerunning all tests (thus also rerunning the full build) fixed it.

@sdedic sdedic merged commit 9ea17f3 into apache:master Jan 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug Bug report or fix Platform [ci] enable platform tests (platform/*)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants