Remove empty lines from view_definition to avoid unecessary recreation of resources

[alvaroqueiroz]

Changes

This PR solves the problem described here: #3330

empty new lines on view_definition cause terraform to plan to change the view definition.
This PR adds the removal of empty lines to DiffSuppressFunc to avoid this problem

Tests

• make test run locally
• relevant change in docs/ folder
• covered with integration tests in internal/acceptance
• relevant acceptance tests are passing
• using Go SDK

[alvaroqueiroz]

hello @mgyucht do you think this is ready for review?

[mgyucht]

This looks good to me. @alexott Do you have any concerns with this?

Only thing is that we should have one integration test to verify this behaves as expected and prevent regression. Look at internal/acceptance/sql_table_test.go. If you install the Databricks CLI (https://github.com/databricks/cli) and login to your workspace, you should be able to run any test. Can you try to add a test for view definition with multiple lines?

[alexott]

My main concern is things like this:

line1\nline2" -> "line1line2

This will break view definitions like this, where it's split between multiple lines:

select col1
from table

will become:

select col1from table

This really should be discussed with the SQL engineering team if it makes sense to do it.

But from the error description, the issue is caused by empty lines, not by newlines, so the replacement should be from this regex \n{1,} -> \n. Also it makes sense to check if it's empty line, vs line with spaces only.

[alvaroqueiroz]

hello @alexott,
this change would not break line1\nline2" -> "line1line2
I have even a test for that

this will remove both, empty lines and empty lines with spaces, both would cause problems

[alvaroqueiroz]

I will add the integration test later today @mgyucht

[alvaroqueiroz]

@mgyucht I have added one acceptance test.
I have added two steps, one creates the view and the second just adds spaces and empty lines to the view definition. It did not generate alter tables as it would with SuppressDiffWhitespaceChange

Let me know if I should make changes to this test

[alvaroqueiroz]

hello @mgyucht
do you think it is worth proceeding with the review of this PR or I should close it?
I just wanted to fix this bug, but if the team thinks this is not the best solution, I'm open to discussing one that fits

[nkvuong]

@alvaroqueiroz could you resolve the merge conflict for this one?

For this change, could you add a unit test that validates there would not be a change when there is diff between InstanceState and HCL?

[alvaroqueiroz]

done @nkvuong
let me know if you need anything else

[nkvuong]

@alvaroqueiroz the conflict resolution for internal/acceptance/sql_table_test.go needs a small fix - could you take a look?

[alvaroqueiroz]

@nkvuong I have synced my fork with the master,
the conflict is still present?

[nkvuong]

@alvaroqueiroz build/fmt is already failing - https://github.com/databricks/terraform-provider-databricks/actions/runs/8466731468/job/23199236255?pr=3361

[alvaroqueiroz]

@nkvuong fixed

[nkvuong]

this test TestResourceSqlTableUpdateView_IgnoreNewlineInDefinition is failing

[alvaroqueiroz]

@nkvuong I think I'm not setting up the test correctly for some reason.
It seems to me that the Update flag will trigger an update regardless of the supressdiff method.

Even If I use the old SuppressDiffWhitespaceChange with just space changes, it will alter the table.
Do you have tips on how I could fix my tests?

[nkvuong]

@alvaroqueiroz for unit testing, you could follow the examples of TestCatalogSuppressCaseSensitivity, which tests that the Diff calculated by Terraform is correct

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.53%. Comparing base (109361e) to head (8811e29).
Report is 8 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3361      +/-   ##
==========================================
+ Coverage   83.51%   83.53%   +0.02%     
==========================================
  Files         178      178              
  Lines       16708    16718      +10     
==========================================
+ Hits        13954    13966      +12     
+ Misses       1901     1900       -1     
+ Partials      853      852       -1     

Files	Coverage Δ
catalog/resource_sql_table.go	89.16% <100.00%> (ø)
common/util.go	100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

[alvaroqueiroz]

just fyi @nkvuong the test is passing because of this line

I will get the test with ExpectedDiff in place

[alvaroqueiroz]

@nkvuong I have tested with ExpectedDiff, I see the same behaviour.
Not even the SuppressDiffWhitespaceChange (the one in prod today) works on the tests

func TestResourceSqlTableUpdateView_IgnoreNewlineInDefinition(t *testing.T) {
	qa.ResourceFixture{
		InstanceState: map[string]string{
			"name":            "barview",
			"catalog_name":    "main",
			"schema_name":     "foo",
			"table_type":      "VIEW",
			"cluster_id":      "existingcluster",
			"view_definition": "SELECT * FROM main.foo.bar",
		},
		ExpectedDiff: map[string]*terraform.ResourceAttrDiff{
			"catalog_name": {
				Old:         "main",
				New:         "main",
				RequiresNew: false,
			},
			"cluster_id": {
				Old:         "existingcluster",
				New:         "existingcluster",
				RequiresNew: false,
			},
			"name": {
				Old:         "barview",
				New:         "barview",
				RequiresNew: false,
			},
			"schema_name": {
				Old:         "foo",
				New:         "foo",
				RequiresNew: false,
			},
			"table_type": {
				Old:         "VIEW",
				New:         "VIEW",
				RequiresNew: false,
			},
			"view_definition": {
				Old:         "SELECT * FROM main.foo.bar",
				New:         "SELECT * FROM main.foo.bar",
				NewComputed: false,
				RequiresNew: false,
				NewRemoved:  false,
				Sensitive:   false,
			},
			"column.#": {
				Old:         "",
				New:         "",
				NewComputed: true,
				RequiresNew: true,
			},
			"properties.%": {
				Old:         "",
				New:         "",
				NewComputed: true,
				RequiresNew: false,
			},
		},
		HCL: `
		name               = "barview"
		catalog_name       = "main"
		schema_name        = "foo"
		table_type         = "VIEW"
		cluster_id         = "existingcluster"
		view_definition    = "SELECT * FROM main.foo.bar "
		`,
		Resource: ResourceSqlTable(),
		ID:       "main.foo.barview",
	}.ApplyNoError(t)
}

[vsluc]

@alvaroqueiroz Appreciate your work. I see a lot of value in this fix, as this unnecessary detection of changes increases the apply time significantly. Also, the recreation of the views with no change is not desirable, as it could run into contention with active ETLs that consume data from the views.