grafanactl: reconcile ADX datasources#228
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new grafanactl modify datasource reconcile-adx subcommand to create/update a single Azure Data Explorer (ADX) Grafana datasource via the Grafana REST API (rather than relying on Azure CLI datasource commands), along with supporting Grafana client capabilities and tests.
Changes:
- Introduces ADX reconcile command + option parsing/validation/completion flow.
- Extends the internal Grafana client with datasource type listing and status-aware create/update requests.
- Updates docs and adds unit tests for the new ADX logic and client behavior.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/grafanactl/internal/grafana/client.go | Adds status-aware datasource create/update helpers and datasource type listing. |
| tools/grafanactl/internal/grafana/client_test.go | Adds tests for create/update request behavior and error reporting. |
| tools/grafanactl/cmd/modify/cmd.go | Registers the new modify datasource reconcile-adx subcommand. |
| tools/grafanactl/cmd/modify/adx_options.go | Implements flags, validation, and completion for ADX reconcile options. |
| tools/grafanactl/cmd/modify/adx.go | Implements reconcile logic (create vs update) and desired datasource payload generation. |
| tools/grafanactl/cmd/modify/adx_options_test.go | Adds unit tests covering validation and reconcile create/update paths. |
| tools/grafanactl/README.md | Documents ADX datasource reconcile usage and common flag alternative. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Replace the temporary shell datasource script with a direct grafanactl invocation and keep cleanup datasource deletion out of this rollout scope. Depends on Azure/ARO-Tools#228.
Replace the temporary shell datasource script with a direct grafanactl invocation and keep cleanup datasource deletion out of this rollout scope. Depends on Azure/ARO-Tools#228.
2b887f7 to
73394d8
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
SudoBrendan
left a comment
There was a problem hiding this comment.
Adding state to our fake client unlocks testing of idempotency (retry) logic of POST. Does this work as expected today?
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
tools/grafanactl/internal/grafana/client.go:221
DeleteDataSourcebuilds the request path viapath.Join("api/datasources/name", url.PathEscape(dataSourceName)).url.PathEscapedoes not escape./.., andpath.Joinwill path-clean those segments (e.g., name".."collapses toapi/datasources), which can send the DELETE to the wrong endpoint. Prefer concatenating the fixed prefix with the escaped name (or building the URL withoutpath.Join/path.Clean) so user-supplied names cannot alter path structure via dot-segments.
func (c *Client) DeleteDataSource(ctx context.Context, dataSourceName string) error {
apiPath := path.Join("api/datasources/name", url.PathEscape(dataSourceName))
if _, err := c.doGrafanaRequest(ctx, http.MethodDelete, apiPath, nil, http.StatusOK, http.StatusAccepted, http.StatusNotFound); err != nil {
return fmt.Errorf("failed to delete datasource %q: %w", dataSourceName, err)
What
Extends the existing
GrafanaDatasourcestyped step andgrafanactl modify datasource reconcileflow with optional ADX/Kusto datasource support.This keeps Azure Monitor datasource reconciliation as the default path, adds ADX flags only when configured, and supports create/update/delete desired-state behavior for the named ADX datasource.
Why
ARO-HCP needs to provision ADX datasources through the existing typed/prebuilt-binary rollout pattern instead of introducing an ADX-only EV2 shell path or rollout-time
go run.Testing
go test ./pipelines/types ./tools/grafanactl/...make lintCross-repo rollout
Merge order is required: