Skip to content

Conversation

@kateknister
Copy link
Contributor

No description provided.

@cadvisorJenkinsBot
Copy link

Can one of the admins verify this patch?

@kateknister kateknister force-pushed the apiBranch branch 3 times, most recently from 8048468 to f882636 Compare February 25, 2015 01:44
@kateknister
Copy link
Contributor Author

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.

@vmarmol
Copy link
Contributor

vmarmol commented Feb 25, 2015

ok to test

api/versions.go Outdated
Copy link
Contributor

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

@vmarmol
Copy link
Contributor

vmarmol commented Feb 25, 2015

Looks good overall :) only minor comments.

api/handler.go Outdated
Copy link
Contributor

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])
}

@vmarmol
Copy link
Contributor

vmarmol commented Feb 26, 2015

Tests look good :)

@kateknister kateknister force-pushed the apiBranch branch 2 times, most recently from 0b67650 to 0dfa447 Compare February 26, 2015 20:15
Copy link
Contributor

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.

@kateknister
Copy link
Contributor Author

I removed the extra test

Copy link
Contributor

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.

@vmarmol
Copy link
Contributor

vmarmol commented Feb 26, 2015

LGTM

kateknister added a commit that referenced this pull request Feb 26, 2015
Added a basic api interface for events
@kateknister kateknister merged commit a210aca into google:master Feb 26, 2015
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.

3 participants