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

Analyse grafana: Use minimal schema #247

Closed
wants to merge 2 commits into from
Closed

Analyse grafana: Use minimal schema #247

wants to merge 2 commits into from

Conversation

hjet
Copy link
Contributor

@hjet hjet commented Mar 22, 2022

The upstream sdk library is now quite out of sync with Grafana and is infrequently updated (as is this tool), and this often results in deserialization errors and breakage (e.g. https://github.com/grafana/support-escalations/issues/2089)

This PR adds a minimal schema which preserves existing functionality of the tool while avoiding all of the data structures and fields not necessary to its functionality. I think it will make this easier to maintain and avoids forking the sdk library.

Deserialization additionally tested on kubernetes-mixin. There are also two dashboards included in unit tests.

Closes #241
Deprecates #239

cc @eamonryan @rgeyer

@hjet hjet changed the title [WIP] analyse grafana: remove SDK dependency Analyse grafana: Use minimal schema Mar 23, 2022
@hjet hjet marked this pull request as ready for review March 23, 2022 18:17
@hjet hjet requested a review from a team as a code owner March 23, 2022 18:17
@@ -0,0 +1,36 @@
package analyse
Copy link
Contributor

Choose a reason for hiding this comment

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

I would write a comment in this mentioning the purpose of these (same thing you wrote on the PR description) to avoid someone coming and completing it with all fields.

}
Target struct {
RefID string `json:"refId"`
Datasource string `json:"datasource,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

This will not parse modern dashboards, where this is an object. So we still need either #239 or my change from grafana-tools/sdk#201

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I don't think this field actually gets used anywhere at the moment, so i could remove it.

However if we fork, I agree that it makes more sense to just use the fork instead of the above approach.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have a fork at https://github.com/colega/grafana-tools-sdk which you can use here as go mod replacement if you need, I've been merging all the fixes into master there.

continue
}

if err = json.Unmarshal(rawBoard, &board); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit:

Suggested change
if err = json.Unmarshal(rawBoard, &board); err != nil {
if err := json.Unmarshal(rawBoard, &board); err != nil {

Even if the outcome is the same, we don't really want to modify the outer err variable.

if err != nil {
return err
}

for _, link := range boardLinks {
board, _, err := c.GetDashboardByUID(ctx, link.UID)
rawBoard, _, err = c.GetRawDashboardBySlug(ctx, link.URI)
Copy link
Contributor

@colega colega Mar 24, 2022

Choose a reason for hiding this comment

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

Nit: I'd keep := here.

@hjet
Copy link
Contributor Author

hjet commented Mar 24, 2022

Closing in favor of #248

@hjet hjet closed this Mar 24, 2022
@hjet hjet deleted the remove_sdk_dep branch March 24, 2022 16:53
friedrichg pushed a commit to cortexproject/cortex-tools that referenced this pull request Aug 1, 2023
* Update Helm release memcached to v5.15.8

Signed-off-by: Renovate Bot <bot@renovateapp.com>

* add changelog entry for dependency update

Signed-off-by: ShuzZzle <niclas.schad@gmail.com>

Co-authored-by: Renovate Bot <bot@renovateapp.com>
Co-authored-by: ShuzZzle <niclas.schad@gmail.com>
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.

[analyse] grafana unmarshal errors
2 participants