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

🐛: Target.GroupBy should be singular in the type declaration #180

Merged
merged 2 commits into from
Nov 9, 2021

Conversation

kevitan
Copy link
Contributor

@kevitan kevitan commented Oct 28, 2021

in Grafana 8.0.5:

Before, I was using the Target.GroupBys property, but it turns out that that did not have any effect. By examining other dashboards manually created, I noticed that the JSON for the Panels actually had groupby in the singular. By changing the panel json for panels created by the grafana sdk and applying it, I was able to get the desired functionality.

With the plural:
Screenshot 2021-10-28 at 16 53 00Screenshot 2021-10-28 at 16 53 13

and with the singular:

Screenshot 2021-10-28 at 16 53 46Screenshot 2021-10-28 at 16 53 31

the name of the cluster is redacted because it does display correctly.

this should restore the groupby functionality in the grafana-sdk. I did a search to see if the GroupBys property was ever used, and I don't see any evidence of it in the documentation.

@coveralls
Copy link

coveralls commented Oct 28, 2021

Coverage Status

Coverage remained the same at 50.311% when pulling 863fe11 on kevitan:bug/target-group-by into 56cdea6 on grafana-tools:master.

@kevitan kevitan changed the title GroupBy should be singular in the JSON Target.GroupBy should be singular in the type declaration Oct 28, 2021
@kevitan kevitan changed the title Target.GroupBy should be singular in the type declaration 🐛: Target.GroupBy should be singular in the type declaration Oct 28, 2021
panel.go Outdated
@@ -566,7 +566,7 @@ type Target struct {
CrossSeriesReducer string `json:"crossSeriesReducer,omitempty"`
PerSeriesAligner string `json:"perSeriesAligner,omitempty"`
ValueType string `json:"valueType,omitempty"`
GroupBys []string `json:"groupBys,omitempty"`
GroupBy []string `json:"groupBys,omitempty"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Doesn't this need to be:

Suggested change
GroupBy []string `json:"groupBys,omitempty"`
GroupBy []string `json:"groupBy,omitempty"`

? At least that's what you say in the PR description if I understood you correctly 🤔

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, that's exactly it. I knew I missed a spot somewhere. I've addressed that now.

@GiedriusS GiedriusS merged commit 6856e18 into grafana-tools:master Nov 9, 2021
K-Phoen added a commit to K-Phoen/sdk that referenced this pull request Nov 19, 2021
wurbanski pushed a commit to kubermatic/grafanasdk that referenced this pull request Mar 30, 2022
…-tools#180)

* GroupBy should be singular in the JSON

* fix json declaration for groupBy

Co-authored-by: Tank <[email protected]>
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