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

internal/v4: implement stats endpoint #52

Merged
merged 2 commits into from Aug 5, 2014

Conversation

rogpeppe
Copy link
Contributor

@rogpeppe rogpeppe commented Aug 4, 2014

internal/v4/stats.go and internal/v4/stats_test.go are directly derived from
code in _old, although simplified to return only JSON-formatted results.

This required a fair number of fixes to other code, which
unfortunately aren't that easy to pick apart now, including the
following:

  • internal/charmstore: make stats counter tests less hacky.

Because the internal/v4 tests are outside charmstore, delving
directly into the database to tweak the time stamps seemed unwarranted,
so added IncCounterAtTime and changed internal/v4 tests and existing
internal/charmstore tests to use it.

  • internal/charmstore: move mongojs condition into storetesting.

This avoids duplicating the logic to determine whether tests requiring
mongodb embedded javascript can be run.

  • internal/router: universal ParseForm.

This clears up responsibility for who should call ParseForm.

  • internal/router: strip prefix from global handlers

This makes the global handlers (including /stats/counters) consistent
with other handlers in router.Handlers.

  • internal/router: add ServeMux, NotFoundHandler

The original /stats/counter tests tested http status of paths which previously
returned an http.StatusNotFound status but with a non-JSON body.
The above two handlers make the not-found pages consistent
with the rest of the API.

  • params: add ErrForbidden, ErrBadRequest

The original /stats/counter tests tested for statuses which our
current code was incapable of generating. The above two
errors make it capable.

internal/charmstore: move mongojs condition into storetesting.

internal/router: universal ParseForm.

internal/router: strip prefix from global handlers

internal/router: add ServeMux, NotFoundHandler
}),
},
},
urlStr: "http://example.com/foo",
urlStr: "http://0.1.2.3/foo",
Copy link
Contributor

Choose a reason for hiding this comment

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

consider using a valid test or documentation IP address range. 198.18/15 is reserved for test. 192.0.2.0/24 is reserved for documentation. See rfc5737 and rfc2544 for more.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, most machines' router tables do not special-case either of the above two ranges, so they don't help to guard against accidental network access from tests. 0.1.2.3, on the other hand is (at least on Linux) picked up immediately by the network stack as an invalid IP address, and generates an error without touching the network.

That's why we use 0.1.2.3 (or similar) as a testing IP address in juju-core too.

@jrwren
Copy link
Contributor

jrwren commented Aug 4, 2014

LGTM

err = counters.Update(filter, bson.D{{"$set", bson.D{{"t", stamp}}}})
c.Check(err, gc.IsNil)
err := s.store.IncCounterAtTime(inc.key, t)
c.Assert(err, gc.IsNil)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Getting rid of hardcoded stuff, nice ;)

@frankban
Copy link
Member

frankban commented Aug 5, 2014

👍 with a few comments.
Thank you!

@rogpeppe
Copy link
Contributor Author

rogpeppe commented Aug 5, 2014

:shipit:

@jujugui
Copy link
Contributor

jujugui commented Aug 5, 2014

Status: merge request accepted. Url: http://ci.jujugui.org:8080/job/charmstore-merge

rogpeppe added a commit that referenced this pull request Aug 5, 2014
internal/v4: implement stats endpoint

manual merge because 'bot is down.
@rogpeppe rogpeppe merged commit a6ce113 into juju:v4 Aug 5, 2014
@jujugui
Copy link
Contributor

jujugui commented Aug 5, 2014

Test FAILed.
Refer to this link for build results: http://ci.jujugui.org:8080/job/charmstore/55/

@jujugui
Copy link
Contributor

jujugui commented Aug 5, 2014

Test FAILed.
Refer to this link for build results: http://ci.jujugui.org:8080/job/charmstore/59/

@jujugui
Copy link
Contributor

jujugui commented Aug 5, 2014

Build failed: Attempt to land pull request failed
build url: http://ci.jujugui.org:8080/job/charmstore-merge/42

@jujugui
Copy link
Contributor

jujugui commented Aug 5, 2014

Test PASSed.
Refer to this link for build results: http://ci.jujugui.org:8080/job/charmstore/62/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants