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

add support for creating, patching, deleting and getting annotations #66

Merged
merged 2 commits into from
Feb 25, 2020

Conversation

pmenglund
Copy link
Contributor

No description provided.

@coveralls
Copy link

coveralls commented Feb 17, 2020

Coverage Status

Coverage increased (+1.8%) to 36.842% when pulling c57c3b4 on rockset:annotations into dcbbf97 on grafana-tools:master.

Copy link
Collaborator

@GiedriusS GiedriusS left a comment

Choose a reason for hiding this comment

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

Just a few minor things but overall LGTM! Also, thank you so much for adding that new param type just like we've talked in the open issue! Much appreciated ❤️

rest-annotation.go Show resolved Hide resolved
rest-annotation.go Show resolved Hide resolved
rest-annotation_integration_test.go Outdated Show resolved Hide resolved
annotation.go Show resolved Hide resolved
@pmenglund
Copy link
Contributor Author

I didn't even see #47 - happy to get you guys started 😄

Copy link
Collaborator

@GiedriusS GiedriusS left a comment

Choose a reason for hiding this comment

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

LGTM! Let's wait for a few days to see if anyone else wants to review (:

@GiedriusS GiedriusS merged commit ab16bd9 into grafana-tools:master Feb 25, 2020
@GiedriusS
Copy link
Collaborator

Thanks! My only little pet peeve of this PR is that in the future we might have other calls with parameters that have the same names so we will have collisions. But, time will tell and that will be refactored (before 1.0) to avoid this problem.

@ankit-arecabay
Copy link

@pmenglund @GiedriusS
This merge is causing a compile error on grafana/sdk package:
github.com/grafana-tools/sdk$ go build

github.com/grafana-tools/sdk

./rest-annotation.go:158:25: syntax error: unexpected _000_000 at end of statement

https://github.com/grafana-tools/sdk/blob/master/rest-annotation.go#L158 --> This looks like an invalid syntax.

@GiedriusS
Copy link
Collaborator

GiedriusS commented Feb 25, 2020

@pmenglund @GiedriusS
This merge is causing a compile error on grafana/sdk package:
github.com/grafana-tools/sdk$ go build

github.com/grafana-tools/sdk

./rest-annotation.go:158:25: syntax error: unexpected _000_000 at end of statement

https://github.com/grafana-tools/sdk/blob/master/rest-annotation.go#L158 --> This looks like an invalid syntax.

I see, thanks for your report! But only on <1.13 versions because in 1.13 support for underscores has been added. I will fix this soon + add extra versions to our CI to catch these in the future.

@GiedriusS
Copy link
Collaborator

@ankit-arecabay #70 PTAL.

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

4 participants