Skip to content

Commit

Permalink
Address feedback from Grafana's plugin review (#25)
Browse files Browse the repository at this point in the history
* Add defer res.Body.Close() for auth HTTP calls

* Remove unnecessary logic in CheckHealth

* Enable release workflow

* Update CHANGELOG and package.json for v1.1.1
  • Loading branch information
dcheckoway authored Nov 5, 2024
1 parent d72f1d0 commit 51153ca
Show file tree
Hide file tree
Showing 5 changed files with 11 additions and 19 deletions.
9 changes: 2 additions & 7 deletions .github/workflows/release.yml
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,5 @@ jobs:
steps:
- uses: actions/checkout@v4
- uses: grafana/plugin-actions/build-plugin@release
# Uncomment to enable plugin signing
# (For more info on how to generate the access policy token see https://grafana.com/developers/plugin-tools/publish-a-plugin/sign-a-plugin#generate-an-access-policy-token)
#with:
# Make sure to save the token in your repository secrets
#policy_token: $
# Usage of GRAFANA_API_KEY is deprecated, prefer `policy_token` option above
#grafana_token: $
with:
policy_token: ${{ secrets.GRAFANA_ACCESS_POLICY_TOKEN }}
5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,8 @@ Initial release. Provides basic data source functionality of fetching saved/sch

- Add support for using dashboard variables in Cribl Search queries.
- Add support for Grafana alerting.

## 1.1.1

- Remove unnecessary logic from CheckHealth
- Properly close HTTP response body in all cases
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "search-datasource",
"version": "1.1.0",
"version": "1.1.1",
"scripts": {
"build": "webpack -c ./.config/webpack/webpack.config.ts --env production",
"dev": "webpack -w -c ./.config/webpack/webpack.config.ts --env development",
Expand Down
2 changes: 2 additions & 0 deletions pkg/plugin/auth.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ func RefreshTokenViaOAuth(criblOrgBaseUrl string, clientId string, clientSecret
if err != nil {
return &BearerToken{}, fmt.Errorf("auth http error: %v", err.Error())
}
defer res.Body.Close()
responseBody, err := io.ReadAll(res.Body)
if err != nil {
return &BearerToken{}, fmt.Errorf("auth error, reading body: %v", err.Error())
Expand Down Expand Up @@ -76,6 +77,7 @@ func RefreshTokenViaLocalAPI(apiBaseUrl string, username string, password string
if err != nil {
return &BearerToken{}, fmt.Errorf("login http error: %v", err.Error())
}
defer res.Body.Close()
responseBody, err := io.ReadAll(res.Body)
if err != nil {
return &BearerToken{}, fmt.Errorf("login error, reading body: %v", err.Error())
Expand Down
12 changes: 1 addition & 11 deletions pkg/plugin/datasource.go
Original file line number Diff line number Diff line change
Expand Up @@ -278,16 +278,6 @@ func (d *Datasource) query(_ context.Context, _ backend.PluginContext, dataQuery
func (d *Datasource) CheckHealth(_ context.Context, req *backend.CheckHealthRequest) (*backend.CheckHealthResult, error) {
res := &backend.CheckHealthResult{}

ps, err := models.LoadPluginSettings(*req.PluginContext.DataSourceInstanceSettings)
if err != nil {
return nil, err
}
// If any of the settings have changed, update the settings and force a fresh auth
if d.Settings.CriblOrgBaseUrl != ps.CriblOrgBaseUrl || d.Settings.ClientId != ps.ClientId || d.Settings.Secrets.ClientSecret != ps.Secrets.ClientSecret {
d.Settings = ps
d.SearchAPI = NewSearchAPI(ps)
}

if !isValidURL(d.Settings.CriblOrgBaseUrl) {
res.Status = backend.HealthStatusError
res.Message = "A valid Cribl Organization URL must be supplied"
Expand All @@ -296,7 +286,7 @@ func (d *Datasource) CheckHealth(_ context.Context, req *backend.CheckHealthRequ

// We test the data source by loading saved search IDs. This ensures the creds
// are valid and we'll be able to make API calls successfully.
_, err = d.SearchAPI.LoadSavedSearchIds()
_, err := d.SearchAPI.LoadSavedSearchIds()
if err != nil {
res.Status = backend.HealthStatusError
res.Message = err.Error()
Expand Down

0 comments on commit 51153ca

Please sign in to comment.