-
-
Notifications
You must be signed in to change notification settings - Fork 7.3k
fix: idn ping errors #6662
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: idn ping errors #6662
Conversation
d8da480 to
0664642
Compare
CommanderStorm
left a comment
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.
could you add a test so that the ping monitor does not backslides on this?
This pr will also fix issue #3929
|
louislam
left a comment
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.
I think your last manual test was not testing ping monitor?
c5d662b to
286bd73
Compare
0c20f0f to
261d772
Compare
CommanderStorm
left a comment
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.
From my side this LGTM now.
It needs an approval from louis for a merge
I had already pushed an update when I noticed your message. |
Removed unconditionally stripping of brackets from destAddr.
Removed IDN to punycode conversion for hostname.


ℹ️ To keep reviews fast and effective, please make sure you’ve read our pull request guidelines
📝 Summary of changes done and why they are done
The purpose of this PR is to allow for adding Ping type monitors with IDN domains.
To achieve this, I have added IDN translation by calling URL(address) and extracting the hostname from the result.
During testing, I became aware of the URL call imposes square brackes around IPv6 addresses.
I have therefore added a block that remove those bracket to make it compatible with a shell ping command.
I have also added a similar IDN translation to the defaultFriendlyName function for monitor.hostname in EditMonitor.js.
This change will show the punycode in the Friendly Name input field in other hostname related monitor types like it is done for the HTTP(s) monitor types.
📋 Related issues
📄 Checklist
Please follow this checklist to avoid unnecessary back and forth (click to expand)
I understand that I am responsible for and able to explain every line of code I submit.
📷 Screenshots or Visual Changes
Punycode will show in the Friendly Name fields for hostname related monitors the same way as HTTP(s) monitor types
There are no other visible changes to the user interface