Skip to content

Conversation

@YashashGaurav
Copy link
Contributor

@YashashGaurav YashashGaurav commented Oct 31, 2025

Changes

This PR fixes a bug where the databricks_dashboard resource would not detect content changes when using the file_path attribute. It only detected changes to the path string itself.

The Problem: When we modified the dashboard JSON file locally, terraform plan would show no changes, and the dashboard would not be updated on apply.

The suggested Solution: This fix applies the same content-hashing CustomSuppressDiff logic (now refactored into customDiffDashboardContent) to the file_path attribute that was already in use for serialized_dashboard.

This ensures that any change to the file's content will trigger a resource update, as intended.

Tests

We used the repository's Makefile targets (make fmt, make lint, make ws) for code quality checks, and the Go test command (go test -v ./dashboards) to run the unit tests for the dashboards package.

  • make test run locally
  • relevant change in docs/ folder
  • covered with integration tests in internal/acceptance
  • using Go SDK
  • using TF Plugin Framework
  • has entry in NEXT_CHANGELOG.md file

@YashashGaurav YashashGaurav requested review from a team as code owners October 31, 2025 01:08
@YashashGaurav YashashGaurav requested review from Divyansh-db and removed request for a team October 31, 2025 01:08
@alexott alexott requested a review from Copilot October 31, 2025 08:43
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes a bug where the databricks_dashboard resource failed to detect content changes when using the file_path attribute, only detecting changes to the path string itself. The fix refactors the existing content-hashing logic into a shared function and applies it to both serialized_dashboard and file_path attributes.

Key changes:

  • Renamed customDiffSerializedDashboard to customDiffDashboardContent and refactored it to read both serialized_dashboard and file_path from the resource data
  • Applied the custom diff suppression logic to the file_path attribute to enable content change detection

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 24 to 27
// Read both serialized_dashboard and file_path from the new config
serializedDashboard := d.Get("serialized_dashboard").(string)
filePath := d.Get("file_path").(string)

Copy link

Copilot AI Oct 31, 2025

Choose a reason for hiding this comment

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

The function ignores the new parameter passed to it and always reads from d.Get(). This means when the file_path attribute changes, the function will still read the old path value from the resource data instead of the new value. Consider using the new parameter when k is 'file_path' to ensure the updated file content is read: if k == \"file_path\" { filePath = new }

Suggested change
// Read both serialized_dashboard and file_path from the new config
serializedDashboard := d.Get("serialized_dashboard").(string)
filePath := d.Get("file_path").(string)
// Use the new value for the attribute being diffed
serializedDashboard := d.Get("serialized_dashboard").(string)
filePath := d.Get("file_path").(string)
// If the diff is for serialized_dashboard or file_path, use the new value
if k == "serialized_dashboard" {
serializedDashboard = new
}
if k == "file_path" {
filePath = new
}

Copilot uses AI. Check for mistakes.
Copy link
Contributor

Choose a reason for hiding this comment

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

@YashashGaurav please address this comment...

Copy link
Contributor

@alexott alexott left a comment

Choose a reason for hiding this comment

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

From quick look, looks good. Two things:

  1. Add NEXT_CHANGELOG entry
  2. Add a unit test that will test that it works for file_path as well

@YashashGaurav
Copy link
Contributor Author

Hey @alexott - Thanks for the feedback. I have added the test as you asked for. Though I am definitely not a go ninja by any means and trying to help out in the best way I can.

I am wary of the fact that the new test has to add a temp file that can then be loaded for testing. If you are okay with that being in the unit tests, all good.

@rauchy rauchy temporarily deployed to test-trigger-is January 5, 2026 12:23 — with GitHub Actions Inactive
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +70 to +130
func TestDashboardCreateWithFilePath(t *testing.T) {
// Create a temporary file with dashboard JSON content
tmpFile, err := os.CreateTemp("", "dashboard-*.json")
assert.NoError(t, err)
defer os.Remove(tmpFile.Name())

_, err = tmpFile.WriteString("serialized_json")
assert.NoError(t, err)
tmpFile.Close()

qa.ResourceFixture{
MockWorkspaceClientFunc: func(w *mocks.MockWorkspaceClient) {
e := w.GetMockLakeviewAPI().EXPECT()
e.Create(mock.Anything, dashboards.CreateDashboardRequest{
Dashboard: dashboards.Dashboard{
DisplayName: "Dashboard name",
WarehouseId: "abc",
ParentPath: "/path",
SerializedDashboard: "serialized_json",
},
}).Return(&dashboards.Dashboard{
DashboardId: "xyz",
DisplayName: "Dashboard name",
SerializedDashboard: "serialized_json_2",
WarehouseId: "abc",
UpdateTime: "2125678",
}, nil)
e.Publish(mock.Anything, dashboards.PublishRequest{
EmbedCredentials: true,
WarehouseId: "abc",
DashboardId: "xyz",
ForceSendFields: []string{"EmbedCredentials"},
}).Return(&dashboards.PublishedDashboard{
EmbedCredentials: true,
WarehouseId: "abc",
DisplayName: "Dashboard name",
RevisionCreateTime: "823828",
}, nil)
e.Get(mock.Anything, dashboards.GetDashboardRequest{
DashboardId: "xyz",
}).Return(&dashboards.Dashboard{
DashboardId: "xyz",
DisplayName: "Dashboard name",
SerializedDashboard: "serialized_json_2",
WarehouseId: "abc",
UpdateTime: "2125678",
}, nil)
},
Resource: ResourceDashboard(),
Create: true,
HCL: fmt.Sprintf(`
display_name = "Dashboard name"
warehouse_id = "abc"
parent_path = "/path"
file_path = "%s"
`, tmpFile.Name()),
}.ApplyAndExpectData(t, map[string]any{
"id": "xyz",
"display_name": "Dashboard name",
})
}
Copy link

Copilot AI Jan 12, 2026

Choose a reason for hiding this comment

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

The new test TestDashboardCreateWithFilePath validates that a dashboard can be created using a file path, but it doesn't test the actual diff detection functionality that this PR is meant to fix. Consider adding a test that:

  1. Creates a dashboard with a file_path
  2. Modifies the file content
  3. Verifies that terraform detects the change (i.e., the customDiffDashboardContent function returns false to show the diff)

This would provide better coverage for the bug fix described in the PR.

Copilot uses AI. Check for mistakes.
Comment on lines 24 to 27
// Read both serialized_dashboard and file_path from the new config
serializedDashboard := d.Get("serialized_dashboard").(string)
filePath := d.Get("file_path").(string)

Copy link
Contributor

Choose a reason for hiding this comment

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

@YashashGaurav please address this comment...

@github-actions
Copy link

If integration tests don't run automatically, an authorized user can run them manually by following the instructions below:

Trigger:
go/deco-tests-run/terraform

Inputs:

  • PR number: 5168
  • Commit SHA: 4960a84bb023efa97e239cb6c99bbe4f78596545

Checks will be approved automatically on success.

@rauchy rauchy temporarily deployed to test-trigger-is January 12, 2026 15:42 — with GitHub Actions Inactive
@rauchy rauchy added this pull request to the merge queue Jan 13, 2026
Merged via the queue into databricks:main with commit 4e949ee Jan 13, 2026
12 checks passed
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.

4 participants