fix(dashboard): preserve unknown panel types on read/write round-trip#2856
Merged
fix(dashboard): preserve unknown panel types on read/write round-trip#2856
Conversation
…path)
Replace the branch in that was silently
stripping panel identity for unrecognized panel types (e.g.
, , ). The new logic:
1. Marshals the raw JSON and extracts uid=501(tobio) gid=20(staff) groups=20(staff),12(everyone),61(localaccounts),79(_appserverusr),80(admin),81(_appserveradm),701(com.apple.sharepoint.group.1),33(_appstore),98(_lpadmin),100(_lpoperator),204(_developer),250(_analyticsusers),395(com.apple.access_ftp),398(com.apple.access_screensharing),399(com.apple.access_ssh),400(com.apple.access_remote_ae), , and
to preserve them in the Terraform state.
2. Stores the raw API config in , which flows through the
existing semantic-defaults machinery ().
3. Clears all typed config blocks to avoid stale data from prior state.
On the write path (), unknown panel types with set
are now reconstructed by building the full panel JSON (type + grid +
id + config) and setting it directly on the union
via .
Also adds a helper to convert float64 from JSON decoding
into *float32 for .
Removes the test case that expected an error for type with
, since this is now handled as a round-tripped unknown
panel type.
- Fix float32Ptr to not discard zero grid values (w=0, h=0 are valid) - Improve toAPI catch-all error to distinguish known vs unknown panel types: unknown types now get a clear 'not yet supported' message - Add read-path test for unknown panel type in Test_mapPanelsFromAPI - Add write-path test for unknown panel type in Test_panelsToAPI - Add Test_unknownPanelRoundTrip verifying round-trip preservation - Add Test_unknownPanelToAPIErrorWithoutConfigJSON for user-authored error
Create a new acceptance test TestAccResourceDashboardUnknownPanel that: 1. Creates a dashboard via the Kibana REST API with a 'discover_session' panel (an unknown panel type that has no typed config block in the provider) 2. Imports the dashboard into Terraform state 3. Verifies the import preserves: panel id, grid (x, y, w, h), type, and config_json 4. Runs a second step with ConfigPlanChecks to verify no-op (empty plan) The test uses a PreConfig function to create the dashboard externally since unknown panel types cannot be authored in Terraform configuration. ImportStateIdFunc is used to dynamically set the import ID from the PreConfig-created dashboard.
Contributor
|
✅ PR Changelog Check passed — |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR aims to prevent state corruption in elasticstack_kibana_dashboard when Kibana dashboards contain panel types the provider doesn’t currently have typed config blocks for, by preserving and round-tripping those panels’ raw config via config_json.
Changes:
- Preserve unknown panel
id,grid,type, and rawconfigintoconfig_jsonon read, and clear any typed config blocks. - On write, if a panel has no typed config block but has preserved
config_json, reconstruct the panel JSON and emit it back to the API. - Add unit tests for unknown panel preservation/round-trip and add an acceptance test intended to validate unknown panel handling.
OpenSpec requirements analysis (delta spec)
- Internal consistency: Inconsistent — REQ-010 still states panel-level
config_jsonis supported only formarkdown/vis, while the updated unknown-panel clause introduces usingconfig_jsonfor unknown types on write (needs wording clarification to distinguish practitioner-authored vs computed/preserved usage). - Implementation compliance: Partially met — the core preservation approach is implemented, but the unknown-panel grid handling can incorrectly force omitted
w/hto0(potentially generating invalid API payloads), and the spec’s “validate/plan” rejection of authored unknown types does not appear to be enforced until apply-time. - Test opportunities: The acceptance test as written does not currently exercise the PR description’s “API-created unknown panel -> import -> no-op plan” flow.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| openspec/changes/fix-dashboard-unknown-panel-preservation/tasks.md | Marks tasks complete; checklist wording appears out of sync with the chosen config_json approach. |
| openspec/changes/fix-dashboard-unknown-panel-preservation/specs/kibana-dashboard/spec.md | Updates REQ-010 to require unknown panel preservation via config_json. |
| openspec/changes/fix-dashboard-unknown-panel-preservation/design.md | Updates design decision to reuse config_json for unknown panel payload storage. |
| internal/kibana/dashboard/testdata/TestAccResourceDashboardUnknownPanel/import/main.tf | Adds import-oriented test config for unknown-panel scenarios. |
| internal/kibana/dashboard/testdata/TestAccResourceDashboardUnknownPanel/create/main.tf | Adds create config used by the new acceptance test. |
| internal/kibana/dashboard/models_panels.go | Implements unknown panel read preservation and write reconstruction logic. |
| internal/kibana/dashboard/models_panels_test.go | Adds unit tests for unknown panel preservation and round-trip behavior. |
| internal/kibana/dashboard/acc_unknown_panels_test.go | Adds an acceptance test around unknown panel handling via out-of-band API mutation. |
…ervation - Add presence checks for grid.w/h (avoid treating missing=0) - Clear id/grid/config_json before populating from API to prevent stale state carry-over in the unknown-panel default branch - Replace manual for-loop with slices.Contains (modernize lint fix) - Wrap long error message string to satisfy lll lint rule - Use clients.CompositeIDFromStr helper in acceptance test - Fix testifylint: require.Empty instead of require.Len(t, x, 0) - Clarify acceptance test comment: plan is non-empty (config vs state diverge), not a no-op, which is the expected and correct behaviour
…rved unknown panels - Distinguish practitioner-authored config_json from provider-populated config_json (set during read for unknown panel type preservation) - Clarify write-path exception: preserved payloads re-emitted without error - Update tasks.md task 1.1 to reflect actual implementation (config_json reuse instead of rawConfig private field)
… verify-openspec Co-Authored-By: Claude Opus 4.6 <[email protected]>
Contributor
|
Commit pushed:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Verification Report: fix-dashboard-unknown-panel-preservation
Summary
| Dimension | Status |
|---|---|
| Completeness | 9/10 tasks done (task 4.2 is archive — handled below); 6/6 spec requirements covered |
| Correctness | All requirements implemented and tested; scenarios match spec |
| Coherence | Implementation follows design decisions; code patterns consistent |
CI Status
All checks passing: Build, Lint, CodeQL, OpenSpec Gate, Provider Gate, Check PR changelog, and all 40+ Matrix Acceptance Tests (8.0.1 through 9.4.0).
Issues by Priority
CRITICAL: None
WARNING: None
SUGGESTION:
internal/kibana/dashboard/testdata/TestAccResourceDashboardUnknownPanel/import/main.tf— unused test fixture (both test steps use"create"directory). Minor cleanup opportunity.
Out-of-scope / unassociated changes
All non-change-artifact files are relevant to the change:
internal/kibana/dashboard/models_panels.go— read/write path implementation (tasks 1.x, 2.x)internal/kibana/dashboard/models_panels_test.go— unit tests (task 3.1)internal/kibana/dashboard/acc_unknown_panels_test.go— acceptance test (task 3.2)internal/kibana/dashboard/testdata/TestAccResourceDashboardUnknownPanel/create/main.tf— acceptance test fixtureinternal/kibana/dashboard/testdata/TestAccResourceDashboardUnknownPanel/import/main.tf— test fixture (unused, but related to the change scope)
No unassociated files.
Final Assessment
All checks passed. Zero CRITICAL issues, zero unassociated files. Ready for archive.
Generated by OpenSpec verify (label) for issue #2856 · ● 949.7K
| @@ -0,0 +1,20 @@ | |||
| variable "dashboard_title" { | |||
Contributor
There was a problem hiding this comment.
SUGGESTION: This test fixture is not referenced by any test step — both steps in TestAccResourceDashboardUnknownPanel use acctest.NamedTestCaseDirectory("create"). Consider removing this file or adding a step that uses the import directory.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes a bug where the dashboard resource's read path silently stripped the
id,grid, and config of any panel type it did not recognize (e.g.,discover_session,image,slo_alerts). Refreshing or importing a dashboard containing such panels corrupted state invisibly.Changes
mapPanelFromAPI): replace the destructivedefaultbranch with logic that preservesid,grid,type, and the full raw API config JSON inconfig_jsonfor unrecognized panel types.config_json, re-marshal that payload into the API request unchanged. When no typed block matches and no preserved payload exists, return the existing unsupported-type error.TestAccResourceDashboardUnknownPanel): creates a dashboard via the Kibana API with an unrecognized panel type, imports it into Terraform, and verifies a no-op plan.Changelog
Customer impact: fix
Summary: Preserve unknown Kibana dashboard panel types during Terraform read/write round-trips.
Spec
OpenSpec change:
fix-dashboard-unknown-panel-preservationExtends REQ-010 (panels and
config_jsonround-trip) to cover unknown panel types. The raw preserved payload is stored in the existingconfig_jsonattribute (alreadyOptional: true, Computed: true), which means it is visible viaterraform showbut not user-authorable for unknown types without an existing preserved payload.Testing
go test ./internal/kibana/dashboard/...✅TestAccResourceDashboardUnknownPanel✅ (7.3s vs live ES 9.4.0 + Kibana)make build✅