-
Couldn't load subscription status.
- Fork 2.4k
Added a basic api interface for events #535
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
Conversation
|
Can one of the admins verify this patch? |
8048468 to
f882636
Compare
|
I'm putting in the streaming write function next, but I tried to break it up a bit by not putting it in this commit. |
|
ok to test |
api/versions.go
Outdated
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.
nit: make v2_0 depend on v1_3
|
Looks good overall :) only minor comments. |
f882636 to
e25510b
Compare
api/handler.go
Outdated
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.
What about we check each of these instead of checking in a loop:
if val, ok := urlMap["all_time"]; ok {
getEventsFromAllTime = strconv.ParseBool(val[0])
}|
Tests look good :) |
0b67650 to
0dfa447
Compare
api/versions_test.go
Outdated
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.
We can probably remove this test, dupe of the above.
0dfa447 to
0744ed1
Compare
|
I removed the extra test |
manager/manager.go
Outdated
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.
Looks like this should be moved outside the switch.
0744ed1 to
6e14267
Compare
|
LGTM |
Added a basic api interface for events
No description provided.