[Internal] Generate Effective Fields #4057

rauchy · 2024-09-30T11:11:26Z

This PR introduces changes to the TFSDK generator to support “effective fields” for server-provided values. When fields are marked with the new proto annotation ServerProposedIfEmpty (name pending final decision, until then the computed annotation is used to indicate these fields, while specific fields are excluded), the generator will create an additional computed field (e.g., Effective<FieldName>) and add two sync functions to ensure proper handling of user-provided and server-determined values.

Generated Struct:

type ResourceModel struct {
    OriginalField    types.String `tfsdk:"original_field" tf:"optional"`
    EffectiveField   types.String `tfsdk:"effective_field" tf:"computed"`
}

Sync Functions:

func (newState *ResourceModel) SyncEffectiveFieldsDuringCreateOrUpdate(plan ResourceModel) {
    newState.EffectiveField = newState.OriginalField
    newState.OriginalField = plan.OriginalField
}

func (newState *ResourceModel) SyncEffectiveFieldsDuringRead(existingState ResourceModel) {
    if existingState.EffectiveField.ValueString() == newState.OriginalField.ValueString() {
        newState.OriginalField = existingState.OriginalField
    }
}

Changes

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

tanmay-db

codewise LGTM, had some questions.

tanmay-db · 2024-10-14T14:41:14Z

.codegen/_openapi_sha

Does this need the new sha? If not we can generate over existing sha (genkit generate-sdk terraform --openapi-spec 0c86ea6dbd9a730c24ff0d4e509603e476955ac5 ` and decouple bumping sha with changes in the PR.

We don't really need them as part of this PR, it was more to give an example of the changes. I'll remove that commit.

tanmay-db · 2024-10-14T14:42:55Z

.codegen/model.go.tmpl

@@ -18,20 +18,74 @@ import (
 	"github.com/databricks/databricks-sdk-go/marshal"
    "github.com/hashicorp/terraform-plugin-framework/types"
 )
+{{- $excluded := dict "ShareInfo"        (list "CreatedAt" "CreatedBy" "UpdatedAt" "UpdatedBy")


I think this will need to be done for rest of the resources as well? If yes then this dictionary might become long and something we have to maintain.

We will soon implement a new annotation (ServerProposedIfEmpty) for these effective fields. This annotation will not be applied to the excluded fields, allowing us to eliminate the exclusion list. As a result, the list will not expand to include other resources.

tanmay-db · 2024-10-14T15:02:18Z

.codegen/model.go.tmpl

+  {{- $type := "" -}}
+  {{- if .Entity.IsString}}{{$type = "String"}}{{end}}
+  {{- if .Entity.IsBool}}{{$type = "Bool"}}{{end}}
+  {{- if .Entity.IsInt64}}{{$type = "Int64"}}{{end}}


Might be good to also add Int32? (types.Int32)

Possibly, but I was only transferring what was already in place, so I wasn't aware of the consequences of making changes that fall outside the scope of this PR.

tanmay-db · 2024-10-14T15:04:00Z

.codegen/model.go.tmpl

+  {{- if .Entity.IsFloat64}}{{$type = "Float64"}}{{end}}
+  {{- if .Entity.IsInt}}{{$type = "Int64"}}{{end}}
+  {{- if .Entity.Enum}}{{$type = "String"}}{{end}}
+  if existingState.Effective_{{.PascalName}}.Value{{$type}}() == newState.{{.PascalName}}.Value{{$type}}() {


Note: .Value returns zero value (eg: "" string if string is null or unknown), is there a case where we would want to differentiate between these? Right now if one is null / one is unknown, then we will compare them to be the same.

Since this occurs solely during Read operations, it can only be null and never unknown, I believe.

tanmay-db · 2024-10-14T15:05:11Z

.codegen/model.go.tmpl

+  {{end}}
+}
+
+func (newState *{{.PascalName}}) SyncEffectiveFieldsDuringRead(existingState {{.PascalName}}) {


For my understanding, where would we be calling these in the read? Asking because in case there is an error in the read, we shouldn't sync.

SyncEffectiveFieldsDuringRead is a function that should be invoked before saving to the state during a Read operation, while SyncEffectiveFieldsDuringCreateOrUpdate should be called before saving to the state during a Create or Update operation. We might consider introducing an interface here to allow this syncing to be done automatically by the Resource. However, I would prefer to postpone this refactoring for the time being.

tanmay-db · 2024-10-14T15:06:03Z

.codegen/model.go.tmpl

+  {{- if .Entity.IsString}}{{$type = "String"}}{{end}}
+  {{- if .Entity.IsBool}}{{$type = "Bool"}}{{end}}
+  {{- if .Entity.IsInt64}}{{$type = "Int64"}}{{end}}
+  {{- if .Entity.IsFloat64}}{{$type = "Float64"}}{{end}}


same for float32

tanmay-db · 2024-10-14T15:07:02Z

.codegen/model.go.tmpl

+  {{- if and .Entity.IsComputed (or .Entity.IsString .Entity.IsBool .Entity.IsInt64 .Entity.IsFloat64 .Entity.IsInt .Entity.Enum) -}}
+  {{- if not (in $excluded .PascalName) -}}
+  {{- $type := "" -}}
+  {{- if .Entity.IsString}}{{$type = "String"}}{{end}}


I am guessing -- we don't support other types like Object, list, map for now and this is to unblock share migration right?

Correct. For the moment, I chose primitives because I didn't see a use case for other options. We should revisit this decision in the future if we encounter any relevant cases.

tanmay-db · 2024-10-14T15:09:21Z

internal/service/jobs_tf/model.go

 	// The canonical identifier for this job.
 	JobId types.Int64 `tfsdk:"job_id" tf:"optional"`
 	// Settings for this job and all of its runs. These settings can be updated
 	// using the `resetJob` method.
 	Settings []JobSettings `tfsdk:"settings" tf:"optional,object"`
 }

+func (newState *BaseJob) SyncEffectiveFieldsDuringCreateOrUpdate(plan BaseJob) {
+


nit: The space here and below can be removed.

Addressed in ddc10d2

tanmay-db · 2024-10-14T15:12:30Z

.codegen/model.go.tmpl

+  {{- range .Fields -}}
+  {{- if and .Entity.IsComputed (or .Entity.IsString .Entity.IsBool .Entity.IsInt64 .Entity.IsFloat64 .Entity.IsInt .Entity.Enum) -}}
+  {{- if not (in $excluded .PascalName) -}}
+  newState.Effective_{{.PascalName}} = newState.{{.PascalName}}


Do we want the _ in the variable names? Also it seems like some places have 2 Effective, highlighted in another comment.

I believe that using an underscore helps differentiate the actual field name, especially in rare edge cases where the field name includes "Effective." Personally, it helps my understanding. However, if using underscores is considered a significant style violation in Golang, we can reconsider our approach.

tanmay-db · 2024-10-14T15:13:57Z

internal/service/sharing_tf/model.go

 	// Name of the share.
 	Name types.String `tfsdk:"name" tf:"optional"`
 	// A list of shared data objects within the share.
-	Objects []SharedDataObject `tfsdk:"objects" tf:"optional"`
+	Objects []SharedDataObject `tfsdk:"object" tf:"optional"`


was this (objects -> object) done upstream?

tanmay-db · 2024-10-14T15:15:43Z

internal/service/jobs_tf/model.go

+	// based on accessible budget policies of the run_as identity on job
+	// creation or modification.
+	EffectiveBudgetPolicyId           types.String `tfsdk:"effective_budget_policy_id" tf:"optional"`
+	Effective_EffectiveBudgetPolicyId types.String `tfsdk:"effective_effective_budget_policy_id" tf:"computed,optional"`


Rare case when the field name upstream itself starts with Effective?

Wonderful :)

Anyway, dropped underscores in ec8415a.

… allow effective values to be implemented

### New Features and Improvements * Add `databricks_registered_model` data source ([#4033](#4033)). * Add data source `databricks_notification_destinations` ([#4087](#4087)). ### Bug Fixes * Fix databricks_cluster_pluginframework data source ([#4097](#4097)). * Mark unity_catalog_provisioning_state as ReadOnly ([#4116](#4116)). * Tolerate invalid keys in `databricks_workspace_conf` ([#4102](#4102)). * force send `read_only` in `databricks_external_location` when it's changed ([#4067](#4067)). * force send `read_only` in `databricks_storage_credential` when it's changed ([#4083](#4083)). ### Documentation * Document `budget_policy_id` in `databricks_pipeline` and `databricks_job` ([#4110](#4110)). * Reformat code examples in documentation ([#4081](#4081)). * Update documentation for `databricks_model_serving` ([#4115](#4115)). * Updates to resource examples ([#4093](#4093)). ### Internal Changes * Add maxItem=1 validator for object types in plugin framework schema ([#4094](#4094)). * Fix acceptance test for `databricks_registered_model` data source ([#4105](#4105)). * Generate Effective Fields ([#4057](#4057)). * Generate Effective Fields ([#4112](#4112)). * Set SDK used in the useragent in context ([#4092](#4092)). * Support adding context in resources and data sources ([#4085](#4085)). * Update plugin framework schema to use ListNestedBlocks ([#4040](#4040)).

…lds (#4270) This PR is a follow-up to #4057 and #4112. We're shifting away from using the "computed" annotation to decide if a field should have "effective field" behavior. Instead, we're adopting the more precise "isServiceProposedIfEmpty" annotation. This lines up better with what we aim to achieve with effective fields, without making all regular computed and non-effective fields generate effective field code. We'll only merge this once the relevant fields are updated accordingly in the OpenAPI spec. ## Changes  ## 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 Co-authored-by: Omer Lachish <rauchy@users.noreply.github.com>

rauchy requested a review from hectorcast-db September 30, 2024 11:23

rauchy marked this pull request as ready for review September 30, 2024 11:23

rauchy requested review from a team as code owners September 30, 2024 11:23

rauchy force-pushed the rauchy/generate-original-fields-for-computed-fields branch from 0c85215 to 4379a58 Compare September 30, 2024 11:32

hectorcast-db approved these changes Oct 2, 2024

View reviewed changes

rauchy force-pushed the rauchy/generate-original-fields-for-computed-fields branch 3 times, most recently from be33436 to 3d7cd32 Compare October 4, 2024 12:45

rauchy force-pushed the rauchy/generate-original-fields-for-computed-fields branch 2 times, most recently from 8d223c2 to 7879ccb Compare October 14, 2024 11:37

rauchy changed the title ~~[Internal] Add another field starting with _Original for each computed field~~ [Internal] Generate Effective Fields Oct 14, 2024

rauchy force-pushed the rauchy/generate-original-fields-for-computed-fields branch from 7879ccb to ae55d9a Compare October 14, 2024 12:59

rauchy requested review from mgyucht and tanmay-db October 14, 2024 13:28

rauchy force-pushed the rauchy/generate-original-fields-for-computed-fields branch from ae55d9a to 84b9be0 Compare October 14, 2024 14:02

tanmay-db approved these changes Oct 14, 2024

View reviewed changes

tanmay-db reviewed Oct 14, 2024

View reviewed changes

rauchy force-pushed the rauchy/generate-original-fields-for-computed-fields branch from 84b9be0 to 11d6f16 Compare October 15, 2024 09:02

add another field starting with Effective_ for each computed field to…

cabc683

… allow effective values to be implemented

rauchy force-pushed the rauchy/generate-original-fields-for-computed-fields branch from 11d6f16 to cabc683 Compare October 15, 2024 09:29

rauchy added 2 commits October 15, 2024 11:41

drop underscore after Effective

ec8415a

reduce unrequired newlines

ddc10d2

rauchy added this pull request to the merge queue Oct 15, 2024

Merged via the queue into main with commit 1f59bfd Oct 15, 2024
7 checks passed

rauchy deleted the rauchy/generate-original-fields-for-computed-fields branch October 15, 2024 10:24

hectorcast-db mentioned this pull request Oct 17, 2024

[Release] Release v1.54.0 #4117

Merged

rauchy mentioned this pull request Nov 26, 2024

[Internal] Use isServiceProposedIfEmpty annotations for effective fields #4270

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Internal] Generate Effective Fields #4057

[Internal] Generate Effective Fields #4057

rauchy commented Sep 30, 2024 •

edited

Loading

tanmay-db left a comment

tanmay-db Oct 14, 2024

rauchy Oct 15, 2024

tanmay-db Oct 14, 2024

rauchy Oct 15, 2024

tanmay-db Oct 14, 2024

rauchy Oct 15, 2024

tanmay-db Oct 14, 2024

rauchy Oct 15, 2024

tanmay-db Oct 14, 2024

rauchy Oct 15, 2024

tanmay-db Oct 14, 2024

tanmay-db Oct 14, 2024

rauchy Oct 15, 2024

tanmay-db Oct 14, 2024

rauchy Oct 15, 2024

tanmay-db Oct 14, 2024

rauchy Oct 15, 2024

tanmay-db Oct 14, 2024

rauchy Oct 15, 2024

tanmay-db Oct 14, 2024 •

edited

Loading

rauchy Oct 15, 2024

rauchy Oct 15, 2024 •

edited

Loading

[Internal] Generate Effective Fields #4057

[Internal] Generate Effective Fields #4057

Conversation

rauchy commented Sep 30, 2024 • edited Loading

Generated Struct:

Sync Functions:

Changes

Tests

tanmay-db 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

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

tanmay-db Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rauchy Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

rauchy commented Sep 30, 2024 •

edited

Loading

tanmay-db Oct 14, 2024 •

edited

Loading

rauchy Oct 15, 2024 •

edited

Loading