-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
feat(geo-widget): backend storage implementation for geo widget #19811
Conversation
Co-authored-by: Bucky Schwarz <hoorayimhelping@users.noreply.github.com>
@stuartcarnie could you take a look at the swagger changes? |
I added @GeorgeMac as a reviewer, please wait for his response before merging. |
c9dbd62
to
d3411d4
Compare
d3411d4
to
33efc5c
Compare
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.
Don't forget to update pkger
as well so this view type can be used in templates. iirc, common pkger files updated for a view type are clone_resource
, parser
, parser_models
, and the parser/service_tests with an added testdata test. @TCL735 is a pro at these changes, you can look at his merged prs for an example.
dashboard.go
Outdated
case ViewPropertyTypeGeo: | ||
var gvw GeoViewProperties | ||
if err := json.Unmarshal(v.B, &gvw); err != nil { | ||
fmt.Printf("Unmarshaling geo view failed. %+v", err) |
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.
feel free to add a similar prefix to the returned error, but remove this line
6b42ef9
to
b8356bd
Compare
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.
Thanks for making those additions, sorry for the delay on the re-review
Co-authored-by: Bucky Schwarz <hoorayimhelping@users.noreply.github.com>
…into feat/geo-widget-backend
rebase of influxdata:master was done manually... A rebase with master of the fork, as suggested by github, is not preferred |
@@ -22,11 +22,12 @@ func TestView_MarshalJSON(t *testing.T) { | |||
wants wants | |||
}{ | |||
{ | |||
name: "xy", |
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.
❓ Is there a reason we are updating tests for xy
in this pr? This seems a bit out of scope for geo.
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.
Yes, I did it because I have been adding extra test case specific to geo as requested. I wanted to make the source file to be more readable and make it obvious what are the individual cases testing. This individual addition means that you will get 'xy' or 'geo' serialization failed message if something goes wrong.
CI will probably fail until #20506 merges and this PR is updated to include it. It's under review now. |
This a first part of a two phase implementation of UI geo/map widget. This PR contains updates to swagger.yml (additions only, non breaking changes) to allow saving of the widget configuration. 'dashboard.go' serialization configuration has been updated accordingly.