Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Internal] Remove TF annotations on TFSDK models #4383

Merged
merged 6 commits into from
Jan 10, 2025

Conversation

rauchy
Copy link
Contributor

@rauchy rauchy commented Jan 8, 2025

Changes

This PR changes how schemas are defined for plugin framework resources and data sources. Instead of encoding the schema attributes in a tf tag, which involves reflection and code that's harder to reason about, structs must now implement the ApplySchemaCustomizations method and declare schema customizations explicitly there. All TFSDK structs will have this method generated for them, and if you want to declare a schema for your own struct, you should do so as follows:

// BEFORE
type TestDeprecatedTfTags struct {
	Foo types.String `tfsdk:"foo" tf:"computed,optional"`
}

// AFTER
type TestDeprecatedTfTags struct {
	Foo types.String `tfsdk:"foo"`
}

func (TestNestedMapTfSdk) ApplySchemaCustomizations(attrs map[string]AttributeBuilder) map[string]AttributeBuilder {
	attrs["foo"] = attrs["foo"].SetComputed().SetOptional()
	return attrs
}

Tests

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

@rauchy rauchy temporarily deployed to test-trigger-is January 8, 2025 16:18 — with GitHub Actions Inactive
@rauchy rauchy temporarily deployed to test-trigger-is January 8, 2025 16:19 — with GitHub Actions Inactive
@rauchy rauchy force-pushed the remove-tf-annotations-on-tfsdk-models branch from 1fe1eb3 to 17098ad Compare January 8, 2025 16:28
@rauchy rauchy temporarily deployed to test-trigger-is January 8, 2025 16:28 — with GitHub Actions Inactive
@rauchy rauchy temporarily deployed to test-trigger-is January 8, 2025 16:30 — with GitHub Actions Inactive
@rauchy rauchy force-pushed the remove-tf-annotations-on-tfsdk-models branch from 17098ad to 62118b5 Compare January 8, 2025 16:32
@rauchy rauchy temporarily deployed to test-trigger-is January 8, 2025 16:32 — with GitHub Actions Inactive
@rauchy rauchy temporarily deployed to test-trigger-is January 8, 2025 16:32 — with GitHub Actions Inactive
@rauchy rauchy force-pushed the remove-tf-annotations-on-tfsdk-models branch from 62118b5 to f81d9ec Compare January 8, 2025 16:36
@rauchy rauchy temporarily deployed to test-trigger-is January 8, 2025 16:37 — with GitHub Actions Inactive
@rauchy rauchy temporarily deployed to test-trigger-is January 8, 2025 16:38 — with GitHub Actions Inactive
@rauchy rauchy temporarily deployed to test-trigger-is January 8, 2025 16:50 — with GitHub Actions Inactive
@rauchy rauchy temporarily deployed to test-trigger-is January 8, 2025 16:51 — with GitHub Actions Inactive
@rauchy rauchy force-pushed the remove-tf-annotations-on-tfsdk-models branch from d62b492 to 10d702a Compare January 9, 2025 09:18
@rauchy rauchy temporarily deployed to test-trigger-is January 9, 2025 09:18 — with GitHub Actions Inactive
@rauchy rauchy temporarily deployed to test-trigger-is January 9, 2025 09:19 — with GitHub Actions Inactive
@rauchy rauchy temporarily deployed to test-trigger-is January 9, 2025 10:01 — with GitHub Actions Inactive
@rauchy rauchy temporarily deployed to test-trigger-is January 9, 2025 10:02 — with GitHub Actions Inactive
@rauchy rauchy requested review from mgyucht and tanmay-db January 9, 2025 10:06
@rauchy rauchy marked this pull request as ready for review January 9, 2025 10:07
@rauchy rauchy requested review from a team as code owners January 9, 2025 10:07
@rauchy rauchy force-pushed the remove-tf-annotations-on-tfsdk-models branch from 60f2b80 to ae1fc5f Compare January 9, 2025 10:17
@rauchy rauchy temporarily deployed to test-trigger-is January 9, 2025 10:17 — with GitHub Actions Inactive
@rauchy rauchy temporarily deployed to test-trigger-is January 9, 2025 10:18 — with GitHub Actions Inactive
@rauchy rauchy temporarily deployed to test-trigger-is January 9, 2025 11:39 — with GitHub Actions Inactive
@rauchy rauchy temporarily deployed to test-trigger-is January 9, 2025 11:40 — with GitHub Actions Inactive
@rauchy rauchy temporarily deployed to test-trigger-is January 9, 2025 11:50 — with GitHub Actions Inactive
@rauchy rauchy temporarily deployed to test-trigger-is January 9, 2025 11:52 — with GitHub Actions Inactive
@alexott
Copy link
Contributor

alexott commented Jan 9, 2025

Imho, it's making it harder to understand from looking into struct what fields are optional/computed/...

@@ -18,19 +18,16 @@ import (
)

type CustomizableSchemaProvider interface {
ApplySchemaCustomizations(cs CustomizableSchema, path ...string) CustomizableSchema
ApplySchemaCustomizations(map[string]AttributeBuilder) map[string]AttributeBuilder
Copy link
Contributor

Choose a reason for hiding this comment

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

If we only expose the underlying map, some customizations are a bit more cumbersome right?

@rauchy rauchy force-pushed the remove-tf-annotations-on-tfsdk-models branch from 3e368ec to 58c5329 Compare January 9, 2025 13:39
@rauchy rauchy temporarily deployed to test-trigger-is January 9, 2025 13:39 — with GitHub Actions Inactive
@rauchy rauchy temporarily deployed to test-trigger-is January 9, 2025 13:40 — with GitHub Actions Inactive
@rauchy
Copy link
Contributor Author

rauchy commented Jan 9, 2025

@alexott you're right, but it does simplify the reflection parts and moving forward, tf annotations will limit us from further describing the schema with things like descriptions, plan modifiers and validators. It's basically a language we invented which will grow in maintenance.

Will it help if we add these as inline comments? For example,

type CreateRepoRequest struct {
	Url types.String `tfsdk:"url"` // tf:optional
}

@alexott
Copy link
Contributor

alexott commented Jan 9, 2025

comment may help.

another question - how would we additionally tune the behavior? Right now, many structs in Go SDK just blindly declare everything as omitempty even for required fields. If such tuning needs to be done in OpenAPI spec, then it will be cumbersome

@rauchy
Copy link
Contributor Author

rauchy commented Jan 9, 2025

@alexott OpenAPI should be the preferred way, as it'll help things stay consistent across the board. However, if that process is problematic, you could still provide a customizeSchema function to ResourceStructToSchema or DataSourceStructToSchema and handle any manual customizations there.

JobSettings_SdkV2{}.ApplySchemaCustomizations(cs, append(path, "new_settings")...)
func (c ResetJob_SdkV2) ApplySchemaCustomizations(attrs map[string]tfschema.AttributeBuilder) map[string]tfschema.AttributeBuilder {
attrs["job_id"] = attrs["job_id"].SetRequired()
attrs["new_settings"] = attrs["new_settings"].SetRequired()
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing list validator for single object

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is fixed now. I had the conditional order wrong and now it's rendering properly. I do wonder how the missing list validator didn't pop up during make diff-schema.

Copy link
Contributor

Choose a reason for hiding this comment

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

Validators may not show up on diff-schema, I don't know if they are part of the JSON representation (because they are code), similar to plan modifiers.

@mgyucht
Copy link
Contributor

mgyucht commented Jan 9, 2025

@alexott Our goal is to ensure that there is no extra customization needed in TF beyond what is in the API spec. The real issue is actually that the OpenAPI spec maintained by teams doesn't reflect reality of their API (defined using Protobuf). We are asking all teams to migrate to the OpenAPI generator to eliminate this discrepancy; then, there should be perfect alignment between the API and SDKs/Terraform.

@rauchy rauchy force-pushed the remove-tf-annotations-on-tfsdk-models branch from 58c5329 to c313cf2 Compare January 9, 2025 16:38
@rauchy rauchy temporarily deployed to test-trigger-is January 9, 2025 16:38 — with GitHub Actions Inactive
Copy link

github-actions bot commented Jan 9, 2025

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: 4383
  • Commit SHA: c313cf223d44ce04ce90b1a5c4f2bafc427b94c1

Checks will be approved automatically on success.

@rauchy rauchy temporarily deployed to test-trigger-is January 9, 2025 16:39 — with GitHub Actions Inactive
Copy link
Contributor

@mgyucht mgyucht left a comment

Choose a reason for hiding this comment

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

LGTM, but I'm just curious if .(tfschema.AttributeBuilder) is needed when assigning the attributebuilder updated with validator to the attribute builder map.

@rauchy
Copy link
Contributor Author

rauchy commented Jan 10, 2025

LGTM, but I'm just curious if .(tfschema.AttributeBuilder) is needed when assigning the attributebuilder updated with validator to the attribute builder map.

Unfortunately that results in a
tfschema.BaseSchemaBuilder does not implement tfschema.AttributeBuilder (missing method BuildDataSourceAttribute)

@rauchy rauchy enabled auto-merge January 10, 2025 09:17
@rauchy rauchy added this pull request to the merge queue Jan 10, 2025
Merged via the queue into main with commit 1411692 Jan 10, 2025
12 checks passed
@rauchy rauchy deleted the remove-tf-annotations-on-tfsdk-models branch January 10, 2025 09:32
tanmay-db added a commit that referenced this pull request Jan 15, 2025
### New Features and Improvements

 * Add `fallback` to `databricks_external_location` ([#4372](#4372)).

### Bug Fixes

 * Send only what is required in Update of `databricks_credential` ([#4349](#4349)).

### Documentation

 * Add `MANAGE` privilege to the relevant UC resources ([#4371](#4371)).
 * Add a note about schema evolution and `databricks_sql_table` ([#4352](#4352)).
 * Add description of `clean_rooms_notebook_task` in `databricks_job` ([#4368](#4368)).
 * Add docs for cluster attributes for clusters assigned to groups ([#4359](#4359)).
 * Add note about use of `azurerm_databricks` for enhanced security settings ([#4393](#4393)).
 * Clarify views sharing in `databricks_share` ([#4378](#4378)).
 * Document job edit_mode ([#4354](#4354)).

### Internal Changes

 * Auto generated customizable schemas ([#4356](#4356)).
 * Disable `terraform fmt` in exporter to workaround build issues ([#4394](#4394)).
 * Fixed redundant optional owners in share resource and bumped Go SDK ([#4396](#4396)).
 * Load auto-generated resources and data sources ([#4367](#4367)).
 * Migrate workflows that need write access to use hosted runners ([#4377](#4377)).
 * Remove TF annotations on TFSDK models ([#4383](#4383)).
 * Restart Cluster before Library Installation ([#4384](#4384)).

### Dependency Updates

 * Bump github.com/zclconf/go-cty from 1.15.1 to 1.16.0 ([#4374](#4374)).
 * Bump github.com/zclconf/go-cty from 1.16.0 to 1.16.1 ([#4397](#4397)).

### Exporter

 * Add exporting of `for_each_task` in jobs ([#4347](#4347)).
 * Add references for missing permissions/grants types ([#4390](#4390)).
 * Support for group assigned clusters ([#4361](#4361)).
 * Use Go SDK to list clusters ([#4385](#4385)).
 * Use `Workspace.Export` from Go SDK for notebooks/files ([#4391](#4391)).
github-merge-queue bot pushed a commit that referenced this pull request Jan 16, 2025
### New Features and Improvements

* Add `fallback` to `databricks_external_location`
([#4372](#4372)).


### Bug Fixes

* Send only what is required in Update of `databricks_credential`
([#4349](#4349)).


### Documentation

* Add `MANAGE` privilege to the relevant UC resources
([#4371](#4371)).
* Add a note about schema evolution and `databricks_sql_table`
([#4352](#4352)).
* Add description of `clean_rooms_notebook_task` in `databricks_job`
([#4368](#4368)).
* Add docs for cluster attributes for clusters assigned to groups
([#4359](#4359)).
* Add note about use of `azurerm_databricks` for enhanced security
settings
([#4393](#4393)).
* Clarify views sharing in `databricks_share`
([#4378](#4378)).
* Document job edit_mode
([#4354](#4354)).


### Internal Changes

* Auto generated customizable schemas
([#4356](#4356)).
* Disable `terraform fmt` in exporter to workaround build issues
([#4394](#4394)).
* Fixed redundant optional owners in share resource and bumped Go SDK
([#4396](#4396)).
* Load auto-generated resources and data sources
([#4367](#4367)).
* Migrate workflows that need write access to use hosted runners
([#4377](#4377)).
* Remove TF annotations on TFSDK models
([#4383](#4383)).
* Restart Cluster before Library Installation
([#4384](#4384)).


### Dependency Updates

* Bump github.com/zclconf/go-cty from 1.15.1 to 1.16.0
([#4374](#4374)).
* Bump github.com/zclconf/go-cty from 1.16.0 to 1.16.1
([#4397](#4397)).


### Exporter

* Add exporting of `for_each_task` in jobs
([#4347](#4347)).
* Add references for missing permissions/grants types
([#4390](#4390)).
* Support for group assigned clusters
([#4361](#4361)).
* Use Go SDK to list clusters
([#4385](#4385)).
* Use `Workspace.Export` from Go SDK for notebooks/files
([#4391](#4391)).
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.

3 participants