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

isup: fix error-series HTTP status codes; get away from assuming TLD #1940

Merged
merged 4 commits into from
Sep 26, 2020

Conversation

dgw
Copy link
Member

@dgw dgw commented Sep 18, 2020

Description

Unbelievably, isup has been sometimes (often?) returning "looks fine" for sites that send back error status codes, e.g. HTTP 4xx, 5xx. Those should be considered "down".

I also threw in a couple other little tweaks, the important one being that a bare name like google will no longer assume google.com as the input. (There's one last TODO left for myself before this PR is actually ready for review, about properly handling full URLs that contain a . but refer to a LAN name.)

Checklist

  • I have read CONTRIBUTING.md
  • I can and do license this contribution under the EFLv2
  • No issues are reported by make qa (runs make quality and make test)
  • I have tested the functionality of the things this change touches

dgw added 2 commits September 17, 2020 13:46
This explains so much about how I'd check a site that's clearly down if
I visit it in my browser and Sopel would say it's fine.
@dgw dgw added Tweak Bugfix Generally, PRs that reference (and fix) one or more issue(s) labels Sep 18, 2020
@dgw dgw added this to the 7.1.0 milestone Sep 18, 2020
The "com" TLD is nowhere near as common as it once was. It's probably
better to just inform the user that they forgot a dot, or tried to look
up a local (unqualified) name that Sopel won't be able to access.

Sopel definitely shouldn't just skip enforcing the presence of a dot,
since LAN names' existence shouldn't be accessible this way. Left a TODO
comment about mitigating this kind of risk.
@Exirel
Copy link
Contributor

Exirel commented Sep 18, 2020

e.g. HTTP 4xx, 5xx. Those should be considered "down".

I understand what you say with that quoted "down", and I'd say we need to inform the end-user about the difference between being unreachable, falling into a server error (5xx), and just a URL that cannot be accessed from here (4xx). For instance, 401/403 means the site requires authentication/authorization, not that it is down. The same way, a 404 may mean that you are trying an URL that doesn't return anything, and yet, the site itself isn't down, you can still access it.

So, maybe we should change the message for these cases instead.

@dgw
Copy link
Member Author

dgw commented Sep 18, 2020

Hmm… That makes a lot of sense, though I was (unconsciously?) associating our .isup command with isup.me/downforeveryoneorjustme.com—which just calls unreachable, timeout, HTTP error, whatever… all "down". It doesn't get specific. Maybe I should "pull an @Exirel" and say let's get more specific in a separate PR? 😜

@Exirel
Copy link
Contributor

Exirel commented Sep 18, 2020

let's get more specific in a separate PR? 😜

Yes!

@dgw dgw marked this pull request as ready for review September 20, 2020 10:57
@dgw dgw requested a review from a team September 20, 2020 10:57
@dgw
Copy link
Member Author

dgw commented Sep 20, 2020

Took care of my last TODO. Ready for a real look. 😸

Copy link
Contributor

@Exirel Exirel left a comment

Choose a reason for hiding this comment

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

As discussed on IRC, I'd have done that differently, and that doesn't change the fact that this implementation works perfectly. So let's 🚢 it!

Includes tests, since that's how I verified that it used to be allowed.
@dgw dgw merged commit a000267 into master Sep 26, 2020
@dgw dgw deleted the isup-tweak branch September 26, 2020 00:10
@dgw dgw mentioned this pull request Oct 1, 2020
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bugfix Generally, PRs that reference (and fix) one or more issue(s) Tweak
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants