[Feature] Skip calling Read after Create/Update operations for notebooks #4173

alexott · 2024-10-30T12:21:04Z

Changes

It's based on #4190.

It was found that the import API returns object_id as a result of its execution, so we don't really need to call the Read operation that will call get-status to fill all attributes (when we create a notebook using the SOURCE format).

The other formats (DBC and JUPYTER) will still call the Read to fill in the missing parameters, such as language.

It also changed the language to optional,computed so we can safely set the value and don't trigger the diff.

This should help when we import a large number of notebooks, i.e., when applying exported resources.

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~~

workspace/resource_notebook.go

This PR adds the ability for a resource to specify that it may not need to call `Read` after `Create` and `Update` operations so we can avoid performing another API call(s). The resource may implement `CanSkipReadAfterCreateAndUpdate` function that can decide if the `Read` operation should be skipped. I decided to move common part from #4173 to make it easier to review

alexott · 2024-11-03T10:56:36Z

@mgyucht I've opened #4190 with changes in common so it will be easier to review. This PR will be used only for notebook changes.

## Changes  This PR adds the ability for a resource to specify that it may not need to call `Read` after `Create` and `Update` operations so we can avoid performing another API call(s). The resource may implement `CanSkipReadAfterCreateAndUpdate` function that can decide if the `Read` operation should be skipped. I decided to move common part from #4173 to make it easier to review ## Tests  - [x] `make test` run locally - [ ] relevant change in `docs/` folder - [ ] covered with integration tests in `internal/acceptance` - [x] relevant acceptance tests are passing - [ ] using Go SDK

It was found that the import API returns `object_id` as a result of its execution, so we don't really need to call get-status to fill all attributes. This should help when we import a large number of notebooks, i.e., when applying exported resources. This also changes `format` and `language` attributes to `optional,computed` to avoid having issues with defaults, and suppress diff.

…ting)

rauchy

I think this PR is doing a bit more than what the title suggests, especially now that #4190 lets us skip calling Read, and this PR is using that. I'm missing some specific Notebook context, so it's tough for me to fully judge if the change is correct. My review is mainly focused on the tests and code, and I don't see any issues there. @mgyucht, do you think you should have another look at it as well?

workspace/resource_notebook.go

Co-authored-by: Omer Lachish <[email protected]>

github-actions · 2024-12-04T12:04:28Z

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: 4173
Commit SHA: 7f1b4e77d40d18a8a725d72b891ddfd5403a087d

Checks will be approved automatically on success.

eng-dev-ecosystem-bot · 2024-12-04T12:04:55Z

Test Details: go/deco-tests/12159341397

alexott · 2024-12-04T12:08:43Z

@rauchy I've added more context to the PR description

mgyucht

I might be misunderstanding parts of this, as databricks_notebook is a pretty complex resource, but I think there are some concerns we need to address before merging. LMK what you think.

mgyucht · 2024-12-18T09:48:21Z

workspace/resource_notebook.go

+			oldFormat := d.Get("format").(string)
+			if oldFormat == "" {
+				source := d.Get("source").(string)
+				// check if `source` is set, and if it is, use file exension to determine format


Suggested change

// check if `source` is set, and if it is, use file exension to determine format

// check if `source` is set, and if it is, use file extension to determine format

mgyucht · 2024-12-18T09:52:45Z

workspace/resource_notebook.go

 			return nil
 		},
 		Read: func(ctx context.Context, d *schema.ResourceData, c *common.DatabricksClient) error {
+			oldFormat := d.Get("format").(string)


To clarify, we are doing this in the Read() method for resource import? We should set these fields correctly on create/update definitely. IIUC we won't have access to the source field during import, so there will be a diff anyways after importing a notebook, and this would be set in the update anyways. So I'm wondering why we need to set these in Read() at all.

mgyucht · 2024-12-18T10:01:09Z

workspace/resource_notebook.go

 				}
 				if err != nil {
 					return err
 				}
 			}
+			if d.Get("object_type").(string) == "" {
+				d.Set("object_type", Notebook)


Why do we need to know the object type here? Isn't it always notebook for a databricks_notebook? Or is it because this can be a DBC file which can contain other stuff as well?

I am also missing when it would be set before (when this if block would be skipped). It's marked as deprecated and should always be notebook, I guess?

mgyucht · 2024-12-18T10:04:05Z

workspace/resource_notebook.go

@@ -272,6 +279,10 @@ func ResourceNotebook() common.Resource {
 				Path:      path,
 				Overwrite: true,
 			}
+			if createNotebook.Format == "" && createNotebook.Language != "" {


Can users specify format? It's not documented on the public docs: https://registry.terraform.io/providers/databricks/databricks/1.61.0/docs/resources/notebook. If not, we can remove the first branch of the if since it is always true.

mgyucht · 2024-12-18T10:07:10Z

workspace/resource_notebook.go

+			if createNotebook.Format == "" && createNotebook.Language != "" {
+				createNotebook.Format = "SOURCE"
+				d.Set("format", createNotebook.Format)
+			}


To make this easier to follow, it seems like we should do something like the following:

if createNotebook.Language == "" { // Users must not specify `content_base64` without a language, so // `source` must be set. ext := strings.ToLower(filepath.Ext(d.Get("source").(string))) createNotebook.Language = extMap[ext].Language createNotebook.Format = extMap[ext].Format // Overwrite cannot be used for Dbc format createNotebook.Overwrite = extMap[ext].Overwrite d.Set("language", createNotebook.Language) } else { createNotebook.Format = "SOURCE" } d.Set("format", createNotebook.Format)

Is this logic correct?

mgyucht · 2024-12-18T10:10:46Z

workspace/resource_notebook_test.go

+			"source": "acceptance/testdata/tf-test-jupyter.ipynb",
+			"format": "JUPYTER",


I don't think these will normally be part of the resource state during read for a newly imported resource. What is this test meant to cover?

mgyucht · 2024-12-18T10:11:00Z

workspace/resource_notebook_test.go

@@ -119,22 +155,8 @@ func TestResourceNotebookCreate_DirectoryExist(t *testing.T) {
 					Overwrite: true,
 					Format:    "SOURCE",
 				},
-			},
-			{


Very nice that we save multiple API calls here!

mgyucht · 2024-12-18T10:13:34Z

workspace/acceptance/testdata/acc-test-update1.ipynb

@@ -0,0 +1,42 @@
+{


Can you add a readme for the .dbc files? It's not clear what is in those.

mgyucht · 2024-12-18T10:14:21Z

workspace/resource_notebook.go

 // ResourceNotebook manages notebooks
 func ResourceNotebook() common.Resource {
 	s := FileContentSchema(map[string]*schema.Schema{
 		"language": {
 			Type:     schema.TypeString,
 			Optional: true,
+			Computed: true, // we need it because it will be filled by the provider or backend


👍 thank you for correcting this

mgyucht · 2024-12-18T10:15:03Z

workspace/resource_notebook.go

-				}
-				ext := strings.ToLower(filepath.Ext(source))
-				return old == extMap[ext].Language
-			},
 		},
 		"format": {
 			Type:     schema.TypeString,
 			Optional: true,


You don't need to change it in this PR, but is this really optional? From the docs, users can't specify this, and it seems like we always set it in Create based on the language/source rather than accepting the user-provided value.

alexott requested review from a team as code owners October 30, 2024 12:21

alexott requested review from rauchy and removed request for a team October 30, 2024 12:21

alexott temporarily deployed to test-trigger-is October 30, 2024 12:21 — with GitHub Actions Inactive

alexott requested a review from mgyucht October 30, 2024 12:21

mgyucht reviewed Oct 30, 2024

View reviewed changes

workspace/resource_notebook.go Outdated Show resolved Hide resolved

workspace/resource_notebook.go Outdated Show resolved Hide resolved

alexott changed the title ~~[Fix] Skip calling get-status for notebooks in Read after Create/Update operations~~ [Feature] Allow to skip calling Read after Create/Update operations Oct 30, 2024

alexott changed the title ~~[Feature] Allow to skip calling Read after Create/Update operations~~ [Feature] WIP: Allow to skip calling Read after Create/Update operations Oct 30, 2024

alexott force-pushed the notebooks-skip-get-status branch from 7ecd15c to 2ddb757 Compare October 30, 2024 17:46

alexott temporarily deployed to test-trigger-is October 30, 2024 17:46 — with GitHub Actions Inactive

alexott changed the title ~~[Feature] WIP: Allow to skip calling Read after Create/Update operations~~ [Feature] WIP: Skip calling Read after Create/Update operations for notebooks Nov 3, 2024

alexott mentioned this pull request Nov 3, 2024

[Internal] Make Read after Create/Update configurable #4190

Merged

5 tasks

alexott force-pushed the notebooks-skip-get-status branch from 2ddb757 to e80cda6 Compare November 5, 2024 13:49

alexott temporarily deployed to test-trigger-is November 5, 2024 13:50 — with GitHub Actions Inactive

select correct format if the source is provided (happens when impor…

7293ae2

…ting)

alexott temporarily deployed to test-trigger-is November 6, 2024 10:26 — with GitHub Actions Inactive

alexott added 2 commits November 19, 2024 13:55

Merge branch 'main' into notebooks-skip-get-status

fafd992

Add integration tests for updating DBC & Jupyter notebooks

7a54068

alexott temporarily deployed to test-trigger-is November 19, 2024 12:59 — with GitHub Actions Inactive

alexott changed the title ~~[Feature] WIP: Skip calling Read after Create/Update operations for notebooks~~ [Feature] Skip calling Read after Create/Update operations for notebooks Nov 19, 2024

alexott temporarily deployed to test-trigger-is November 19, 2024 13:00 — with GitHub Actions Inactive

alexott requested a review from mgyucht November 26, 2024 10:13

rauchy approved these changes Dec 4, 2024

View reviewed changes

workspace/resource_notebook.go Outdated Show resolved Hide resolved

Update workspace/resource_notebook.go

7f1b4e7

Co-authored-by: Omer Lachish <[email protected]>

alexott temporarily deployed to test-trigger-is December 4, 2024 12:04 — with GitHub Actions Inactive

mgyucht requested changes Dec 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature] Skip calling Read after Create/Update operations for notebooks #4173

[Feature] Skip calling Read after Create/Update operations for notebooks #4173

alexott commented Oct 30, 2024 •

edited

Loading

alexott commented Nov 3, 2024

rauchy left a comment

github-actions bot commented Dec 4, 2024

eng-dev-ecosystem-bot commented Dec 4, 2024

alexott commented Dec 4, 2024

mgyucht left a comment

mgyucht Dec 18, 2024

mgyucht Dec 18, 2024

mgyucht Dec 18, 2024

mgyucht Dec 18, 2024

mgyucht Dec 18, 2024

mgyucht Dec 18, 2024

mgyucht Dec 18, 2024

mgyucht Dec 18, 2024

mgyucht Dec 18, 2024

mgyucht Dec 18, 2024

	// check if `source` is set, and if it is, use file exension to determine format
	// check if `source` is set, and if it is, use file extension to determine format

		"source": "acceptance/testdata/tf-test-jupyter.ipynb",
		"format": "JUPYTER",

[Feature] Skip calling Read after Create/Update operations for notebooks #4173

Are you sure you want to change the base?

[Feature] Skip calling Read after Create/Update operations for notebooks #4173

Conversation

alexott commented Oct 30, 2024 • edited Loading

Changes

Tests

alexott commented Nov 3, 2024

rauchy left a comment

Choose a reason for hiding this comment

github-actions bot commented Dec 4, 2024

eng-dev-ecosystem-bot commented Dec 4, 2024

alexott commented Dec 4, 2024

mgyucht left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexott commented Oct 30, 2024 •

edited

Loading