-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Cleaner and more straightforward version when using proxy server #2728
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
base: 3.2
Are you sure you want to change the base?
Conversation
Hi @w4tun, thanks. Actually I was only wondering why we didn't set it to true. Can you comment on the other changes please other than "cleaner and more straightforward approach"? FYI: It might not be the right time yet to merge this PR as this point of time I don't know yet whether want a breaking change. |
38610a6
to
0e90d55
Compare
Thanks! Some days after Easter |
I have tried:
Don't know what I did wrong. |
Did you try this PR? Because it says "3.1dev" which is a branch from the past |
|
Hi @drwetter and @michael-o . The main branch in my repo is called "3.1dev" because that was the name in the past when I forked it, but it is currently up to date with 3.2 branch. I will rename my branch to avoid confusion. @michael-o Please try the following command and let me know if it works:
With this new PR, the Please note that this is a breaking change and that's why Dirk needs to review it, but let me know if it works for your test case. Cheers. |
Unfortunately not:
|
Hi @michael-o . I have just sent a new commit to my repo. I must have messed something up when I rebased the PR and there was a bug in determine_ip_addresses(). If you can pull the updated version and run again the command, I would really appreciate it:
|
This one is still off:
"Via Proxy" looks mangled and the OpenSSL-related output as well.
and
|
…resolves to multiple addresses
@michael-o It seems there was an issue in the code when the proxy hostname resolved to multiple IP addresses. I have created a new commit which chooses the first IP from the DNS result. Let me know if that solves the problem. |
But what if that first IP isn't reachable, shouldn't you try the next one? |
Checked the last commit. It looks much better now. It shows "Via Proxy" for the first address only, but the rest of the test does work as expected. |
@@ -22232,9 +22235,9 @@ check_proxy() { | |||
else | |||
# We check now preferred whether there was an IPv4 proxy via DNS specified | |||
# If it fails it could be an IPv6 only proxy via DNS or we just can't reach the proxy | |||
PROXYIP="$(get_a_record "$PROXYNODE" 2>/dev/null | grep -v alias | sed 's/^.*address //')" | |||
PROXYIP="$(get_a_record "$PROXYNODE" 2>/dev/null | grep -v alias | sed 's/^.*address //' | head -n1)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well: "arg1: a host name. Returned will be 0-n IPv4 addresses"
If you really want to use the first IP, one should issue a warning, that only the first IP address is taken into account...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, I definitely need to review this part. I need to do more tests.
Looks like there's some work left. I'd rather do the release and then I can look into this. @w4ntun please tick the boxes accordingly. Formally I should not merge this. |
@drwetter We definitely need to review this because there are still some issues left and we need to do more testing. I have marked the PR as breaking change in the checkboxes. |
Describe your changes
Please refer to an issue here or describe the change thoroughly in your PR: #2719
What is your pull request about?
If it's a code change please check the boxes which are applicable
help()