Skip to content

Support both html and json output for statuses #5

Merged
merged 1 commit into from Dec 4, 2013

6 participants

@sosedoff

Site now supports status information for both HTML and JSON outputs. I added extra route /status that points to the same method.

JSON will be returned at /status.json in the following format:

{
  "status": "up",
  "services": [{
    "name": "Application",
    "description": "Main application",
    "status": "up",
    "last_update": "2012-10-24T06:05:12Z"
  }]
}
@pivotalcommon

We would love to see this as well

@adkron
adkron commented Dec 5, 2012

This appears to be quite similar to #7. Is there something I'm missing other than the format returned?

@sosedoff
sosedoff commented Dec 5, 2012

Small difference, but this PR have been sitting here for a while, seems like nobody is interested.

@adkron
adkron commented Dec 5, 2012

Does the other one meet your needs?

@sosedoff
sosedoff commented Dec 5, 2012

You better ask @hone or @sferik, they're merging things here, but im fine either way.

@sferik
RubyGems member
sferik commented Dec 5, 2012

This looks good to me, but I'll defer to @hone to pull and deploy it. Obviously, we don't want to publish a public API and then break it in the future, so it's worth putting in a little extra time and thought to get it right the first time. I'm not sure if Terence has particular opinions or plans for the API. We'll try to get something live soon, I promise.

@gsiener
gsiener commented Dec 27, 2012

Any updates on this? Hoping to pull individual statuses w/o having to parse the status tables.

Thanks!

@gsiener
gsiener commented Jan 15, 2013

Hi @hone, we've integrated RubyGems status into Project Monitor. Right now we're just scraping the html status page, any thoughts on whether we can merge this in?

@dwradcliffe
RubyGems member

This looks good to me. Anyone have any objections?

@sosedoff
sosedoff commented Dec 4, 2013

Wow, this PR has been sitting here for more than a year ;)

@dwradcliffe
RubyGems member

@sosedoff Yep! Time to merge or close.

@sferik sferik merged commit c2b3a5b into rubygems:master Dec 4, 2013
@sferik
RubyGems member
sferik commented Dec 4, 2013

Time to merge!

@sferik
RubyGems member
sferik commented Dec 4, 2013

Wait! No tests! Unmerge!

Seriously, there should be tests for this new endpoint.

@adkron
adkron commented Dec 4, 2013

I agree with @sferik we need to make sure there are tests for everything here. This system is something that a lot of people are dependent on.

@sferik sferik commented on the diff Dec 4, 2013
app/models/ping.rb
@@ -19,4 +19,13 @@ def state
return "unknown" if unknown?
status
end
+
+ def as_json(options={})
+ {
+ :name => service,
+ :description => description,
+ :status => status,
+ :last_update => last_seen
@sferik
RubyGems member
sferik added a note Dec 4, 2013

Since this returns a time, perhaps it should follow the Rails convention of being named something like updated_at?

@dwradcliffe
RubyGems member
dwradcliffe added a note Dec 4, 2013

agree

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@sferik sferik added a commit that referenced this pull request Dec 4, 2013
@sferik sferik Revert "Support both html and json output for statuses"
This reverts commit b88078d because it
did not include any tests. I merged it hastily because it had been
sitting dormant for over a year. #5
ccae977
@dwradcliffe
RubyGems member

No tests probably because we just merged the first tests this morning. But yes, we should add some!

@sosedoff
sosedoff commented Dec 4, 2013

I can add tests for sure

@sferik
RubyGems member
sferik commented Dec 4, 2013

Apparently, there is no way to re-open a pull request once it has been merged. /cc @github

I’m 👍 on this patch—in fact, I merged once—but think we shouldn’t :shipit: until there are tests. I appologize for my hasty clicking of the merge button. I was frustrated after a year of inaction and didn’t take time to re-review the code.

@sosedoff Anyway, if you open a new pull request with tests, I will happily merge it. 😄

@sosedoff sosedoff deleted the unknown repository branch Dec 4, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.