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

feat(geo-widget): backend storage implementation for geo widget #19811

Merged
merged 27 commits into from
Jan 13, 2021

Conversation

dubsky
Copy link
Contributor

@dubsky dubsky commented Oct 23, 2020

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.

  • CHANGELOG.md updated with a link to the PR (not the Issue)
  • Well-formatted commit messages
  • Rebased/mergeable
  • Tests pass
  • http/swagger.yml updated (if modified Go structs or API)
  • Feature flagged (if modified API)
  • Documentation updated or issue created (provide link to issue/pr)
  • Signed CLA (if not already signed)

@dubsky dubsky requested a review from a team as a code owner October 23, 2020 12:10
@dubsky dubsky requested review from Palakp41 and removed request for a team October 23, 2020 12:10
@timhallinflux timhallinflux changed the title feat(geo-widget): backend storage implementation for chronograf geo w… feat(geo-widget): backend storage implementation for geo widget Oct 24, 2020
http/swagger.yml Outdated Show resolved Hide resolved
Co-authored-by: Bucky Schwarz <hoorayimhelping@users.noreply.github.com>
@russorat russorat requested review from a team and stuartcarnie and removed request for a team October 28, 2020 15:42
@russorat
Copy link
Contributor

@stuartcarnie could you take a look at the swagger changes?

@stuartcarnie stuartcarnie removed their request for review October 28, 2020 18:44
@aanthony1243
Copy link
Contributor

I added @GeorgeMac as a reviewer, please wait for his response before merging.

http/swagger.yml Outdated Show resolved Hide resolved
Copy link
Contributor

@glinton glinton left a 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)
Copy link
Contributor

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

@dubsky dubsky requested a review from a team as a code owner November 25, 2020 17:33
Copy link
Contributor

@glinton glinton left a 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

@ivankudibal
Copy link
Contributor

rebase of influxdata:master was done manually... A rebase with master of the fork, as suggested by github, is not preferred

@dubsky dubsky removed the request for review from Palakp41 December 21, 2020 09:09
@@ -22,11 +22,12 @@ func TestView_MarshalJSON(t *testing.T) {
wants wants
}{
{
name: "xy",
Copy link
Contributor

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.

Copy link
Contributor Author

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.

@dubsky dubsky requested a review from Palakp41 December 22, 2020 10:13
@danxmoran
Copy link
Contributor

CI will probably fail until #20506 merges and this PR is updated to include it. It's under review now.

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

9 participants