Skip to content

Added a basic api interface for events#535

Merged
kateknister merged 1 commit into
google:masterfrom
kateknister:apiBranch
Feb 26, 2015
Merged

Added a basic api interface for events#535
kateknister merged 1 commit into
google:masterfrom
kateknister:apiBranch

Conversation

@kateknister

Copy link
Copy Markdown
Contributor

No description provided.

@cadvisorJenkinsBot

Copy link
Copy Markdown

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
Copy Markdown
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

vmarmol commented Feb 25, 2015

Copy link
Copy Markdown
Contributor

ok to test

Comment thread api/versions.go Outdated

Copy link
Copy Markdown
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

vmarmol commented Feb 25, 2015

Copy link
Copy Markdown
Contributor

Looks good overall :) only minor comments.

Comment thread api/handler.go Outdated

Copy link
Copy Markdown
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

vmarmol commented Feb 26, 2015

Copy link
Copy Markdown
Contributor

Tests look good :)

@kateknister kateknister force-pushed the apiBranch branch 2 times, most recently from 0b67650 to 0dfa447 Compare February 26, 2015 20:15
Comment thread api/versions_test.go Outdated

Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

I removed the extra test

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