Skip to content

Add #pretty_print which prints a nice table of data #37

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

Merged
merged 7 commits into from
Feb 16, 2016
Merged

Conversation

parkr
Copy link
Contributor

@parkr parkr commented Jan 28, 2016

This PR introduces a Printer which can print either the old way or the new way. The new way is a nice table:

Domain      | http://parkr.github.io/ was health checked
------------|--------------------------------------------------
State       | valid - is served by Pages
Record Type | CNAME, should be CNAME
IP Problems | not apex domain
Proxied     | yes, through unknown
Domain      | *.github.com/io domain

Domain      | http://ben.balter.com/ was health checked
------------|--------------------------------------------------
State       | valid - is served by Pages
Record Type | CNAME, should be A record
IP Problems | none
Proxied     | yes, through unknown

Domain      | http://benbalter.com/ was health checked
------------|--------------------------------------------------
State       | valid - is served by Pages
Record Type | A, should be A record
IP Problems | none
Proxied     | yes, through unknown

Domain      | http://byparker.com/ was health checked
------------|--------------------------------------------------
State       | invalid - is NOT served by Pages
Reason      | Domain does not resolve to the GitHub Pages server
Record Type | A, should be A record
IP Problems | none

Domain      | http://jekyllrb.com/ was health checked
------------|--------------------------------------------------
State       | valid - is served by Pages
Record Type | A, should be A record
IP Problems | none
Proxied     | yes, through CloudFlare

Domain      | http://import.jekyllrb.com/ was health checked
------------|--------------------------------------------------
State       | valid - is served by Pages
Record Type | A, should be CNAME
IP Problems | not apex domain
Proxied     | yes, through CloudFlare

Domain      | http://oinaefnoieaf.com/ was health checked
------------|--------------------------------------------------
DNS         | does not resolve
State       | invalid - is NOT served by Pages
Reason      | Domain's DNS record could not be retrieved
Record Type | other, should be CNAME
IP Problems | not apex domain

What do you think?

/cc @github/pages

@parkr
Copy link
Contributor Author

parkr commented Jan 28, 2016

@charliesome: I discovered a bug in HealthCheck#apex_domain. In the case of ben.balter.com., our check says that it, wrongly, is an apex domain:

>> Resolv::DNS.open { |dns|
?>   dns.getresources("ben.balter.com.", Resolv::DNS::Resource::IN::NS)
>> }
=> [#<Resolv::DNS::Resource::IN::NS:0x007fec0a5821d8 @name=#<Resolv::DNS::Name: ns1.dnsimple.com.>, @ttl=3599>, 
    #<Resolv::DNS::Resource::IN::NS:0x007fec0a581260 @name=#<Resolv::DNS::Name: ns2.dnsimple.com.>, @ttl=3599>, 
    #<Resolv::DNS::Resource::IN::NS:0x007fec0a580310 @name=#<Resolv::DNS::Name: ns3.dnsimple.com.>, @ttl=3599>, 
    #<Resolv::DNS::Resource::IN::NS:0x007fec0a57b338 @name=#<Resolv::DNS::Name: ns4.dnsimple.com.>, @ttl=3599>]
>> should_be_apex = _.any?
=> true

That method should return false for this subdomain.

@parkr parkr added this to the v0.7.0 milestone Jan 28, 2016
@haileys
Copy link
Contributor

haileys commented Jan 29, 2016

@parkr Yup, you're right. Looks like NS records for benbalter.com are being returned in a query for ben.balter.com. Nasty:

λ dig ns ben.balter.com

; <<>> DiG 9.8.3-P1 <<>> ns ben.balter.com
;; global options: +cmd
;; Got answer:
;; ->>HEADER<<- opcode: QUERY, status: NOERROR, id: 3036
;; flags: qr rd ra; QUERY: 1, ANSWER: 5, AUTHORITY: 0, ADDITIONAL: 0

;; QUESTION SECTION:
;ben.balter.com.            IN  NS

;; ANSWER SECTION:
ben.balter.com.     3600    IN  CNAME   benbalter.com.
benbalter.com.      3600    IN  NS  ns4.dnsimple.com.
benbalter.com.      3600    IN  NS  ns1.dnsimple.com.
benbalter.com.      3600    IN  NS  ns2.dnsimple.com.
benbalter.com.      3600    IN  NS  ns3.dnsimple.com.

@spraints
Copy link
Member

ben.balter.com.     3600    IN  CNAME   benbalter.com.

That doesn't look right. Shouldn't ben.balter.com be a CNAME to benbalter.github.io?

@benbalter
Copy link
Contributor

That doesn't look right. Shouldn't ben.balter.com be a CNAME to benbalter.github.io?

Ha. It's a long story. I'm an edge case. But yes, ben.balter.com is CNAMEd to benbalter.com, which is an ALIAS to benbalter.github.io.

@parkr
Copy link
Contributor Author

parkr commented Jan 29, 2016

@benbalter Well, ok. I think it'll work in the vast majority of cases then. How do you like my new table? I think it reads more easily than the #to_s we have now. :)

ip_problems << "not apex domain" if not values[:apex_domain?]
ip_problems << "invalid domain" if not values[:valid_domain?]
ip_problems << "old ip address used" if values[:old_ip_address?]
output.puts new_line "IP Problems", "#{ip_problems.size > 0 ? ip_problems.join(", ") : "none"} "
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should this only be printed if :should_be_a_record? is true? Or always print it?

@benbalter
Copy link
Contributor

@parkr Nice work. A few thoughts:

  1. I feel like we're adding some unnecessary chrome here that distracts from the purpose (e.g., is anything lost if we remove 'was health checked')?
  2. Have you looked at Terminal Table? It may help simplify things / make things a bit more readable. I'm having trouble following some of the logic.
  3. I suspect a lot of the code could be simplified by a hash_symbol to human-readable-output map. E.g., for many of the variables, the logic is just, if X, display the human readable version of X.

@parkr
Copy link
Contributor Author

parkr commented Feb 2, 2016

I feel like we're adding some unnecessary chrome here that distracts from the purpose

The purpose of this PR is to improve upon the readability of #to_s, which is puts the most important stuff at the bottom. This simply improves upon the display and uses conditionals so only the important stuff is present.

Have you looked at Terminal Table

Yeah, but I didn't want to add another dependency. If you think it's ok, then I can use that instead.

I suspect a lot of the code could be simplified by a hash_symbol to human-readable-output map.

Yes, this function encapsulates the various nuances of that map, e.g. when Y influences the map value of X. It's not straight 1-1 mapping so I didn't go that route. I could create a TablePrinter or use Terminal Table to simplify the formatting logic if you'd like.

@parkr parkr mentioned this pull request Feb 12, 2016
1 task
@benbalter
Copy link
Contributor

Merged in master, which contains #39. Easy peasy.

@@ -56,6 +56,7 @@ raises GitHubPages::HealthCheck::Errors::InvalidCNAMEError
:pages_domain?=>false,
:valid?=>true
}
> require 'json'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@benbalter is this needed still?

Copy link
Contributor

Choose a reason for hiding this comment

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

As currently coded, yes, JSON isn't required by default, but a method calls to_json, which would result in an error if not included. My thinking was, if you require json, to_json would work (just like for native objects), and if you don't you'll get an undefined method to_json error. Does that make sense? (use case being a simple API, that could theoretically just call to_hash.to_json itself).

@@ -9,8 +9,6 @@
require "resolv"
require "timeout"
require "octokit"
require "json"
require "yaml"
Copy link
Contributor

Choose a reason for hiding this comment

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

If using the YAML approach for to_s, you'll want to re-require yaml (or maybe lazy require it in the method?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added in e36ec9b

@parkr
Copy link
Contributor Author

parkr commented Feb 16, 2016

@benbalter Good to merge?

@benbalter
Copy link
Contributor

:shipit:

parkr added a commit that referenced this pull request Feb 16, 2016
Add #pretty_print which prints a nice table of data
@parkr parkr merged commit f9af07d into master Feb 16, 2016
@parkr parkr deleted the better-printer branch February 16, 2016 22:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants