-
Notifications
You must be signed in to change notification settings - Fork 70
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
Switch to grafana-openapi-client-go #262
Conversation
Hi there @mbarrien, Thanks for your effort on helping to migrate to the new client. I think it'd be awesome to get this pull request ready to be merged. So, beyond an eventual code review, now I'm going over all the failing tests (the ones you mentioned), case by case, and trying to fix them (see for instance 👉🏻 grafana/grafana#76749). Once ready, I'll create a PR against your branch, so hopefully you'll branch will be ready to be merged after that. Once more, thanks for your contributions, I think we're getting closer to finally have the "good" client in place! 🙌🏻 |
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 this, much appreciated. A few small comments/questions, but I think we can approve merge this quite soon.
if err != nil { | ||
return nil, err | ||
} | ||
// Losing id, typeLogoUrl, version, withCredentials |
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 this line needed? Or suggests something still needs doing?
} | ||
return data, nil | ||
} | ||
// Losing a bunch of omitempty fields |
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.
unnecessary comment?
Switches datasource, dashboard, and folder resources to using the grafana-openapi-client-go instead of accessing the REST API's directly. This should supersede #260 (and possibly #257 since it adds a field to provider for the OpenAPI client)
There are some small issues:
omitempty
annotations, which results in many fields no longer being present in the next pull after you change to this code (especially for datasources).make test
:id
field is expected to be a string, but Grafana is return a numberNote that some of the functionality lost in #260 due to missing fields has been restored since grafana-openapi-client-go contains them all.
If you have previously run grizzly pull, then run grizzly pull again in the new code, you will get diffs like this on all previously pulled resources (anonymized from actual examples):
Datasources:
Dashboards:
Folders: