Skip to content

Conversation

@bflad
Copy link
Contributor

@bflad bflad commented Jun 22, 2022

Reference: #81
Reference: #161
Reference: #172
Reference: #365

This introduces a native abstraction over terraform-plugin-go's tftypes.AttributePath, allowing the framework to own the implementation details and extend the functionality further. Provider developers will be closer to removing a direct dependency on terraform-plugin-go. This is a major breaking change, however it is necessary before 1.0 to prevent compatibility issues in the future and long has been a goal of the framework.

This functionality is only intended to replace tftypes.AttributePath usage and not mess with the tfsdk.Config, tfsdk.Plan, and tfsdk.State type tftypes.Value data storage fields or their underlying data manipulation logic. This does leave the framework in an awkward half-state until those are further refactored (likely towards native attr.Value), but is done to try and reduce the review complexity of this initial migration. Additional followup changes will introduce the concept of path expressions, which will allow provider developers to match against multiple paths or reference parent paths in schema-based plan modifier and validation functionality.

To prevent import cycles between attr and diag packages due changing the attr.TypeWithValidate type Validate method signature from Validate(context.Context, tftypes.Value, *tftypes.AttributePath) diag.Diagnostics to Validate(context.Context, tftypes.Value, path.Path) diag.Diagnostics, the TypeWithValidation interface is moved a new xattr package underneath attr with Go and website documentation to direct provider developers to the other package. This will also solve a prior issue with trying to implement TypeWithModifyPlan support.

Naming and location of anything in this initial implementation can be adjusted as necessary.

Provider developers can migrate to the new path handling by replacing:

tftypes.NewAttributePath().WithAttributeName("example")

With the new github.com/hashicorp/terraform-plugin-framework/path import equivalent:

path.Root("example")

Then, if necessary, using the (Path).At* methods to extend the path definition instead of (*tftypes.AttributePath).With* methods:

Current New
WithAttributeName() AtName()
WithElementKeyInt() AtListIndex()
WithElementKeyString() AtMapKey()
WithElementKeyValue() AtSetValue()

Reference: #81
Reference: #161
Reference: #215
Reference: #365

This introduces a native abstraction over terraform-plugin-go's `tftypes.AttributePath`, allowing the framework to own the implementation details and extend the functionality further. Provider developers will be closer to removing a direct dependency on terraform-plugin-go. This is a major breaking change, however it is necessary before 1.0 to prevent compatibility issues in the future.

This functionality is only intended to replace `tftypes.AttributePath` usage and not mess with the `tfsdk.Config`, `tfsdk.Plan`, and `tfsdk.State` type `tftypes.Value` data storage fields or their underlying data manipulation logic. This does leave the framework in an awkward half-state until those are further refactored (likely towards native `attr.Value`), but is done to try and reduce the review complexity of this initial migration. Additional followup changes will introduce the concept of path expressions, which will allow provider developers to match against multiple paths or reference parent paths in schema-based plan modifier and validation functionality.

To prevent import cycles between `attr` and `diag` packages due changing the `attr.TypeWithValidate` type `Validate` method signature from `Validate(context.Context, tftypes.Value, *tftypes.AttributePath) diag.Diagnostics` to `Validate(context.Context, tftypes.Value, path.Path) diag.Diagnostics`, the `TypeWithValidation` interface is moved a new `xattr` package underneath `attr` with Go and website documentation to direct provider developers to the other package. This will also solve a prior issue with trying to implement `TypeWithModifyPlan` support.

Naming and location of anything in this initial implementation can be adjusted as necessary.

Provider developers can migrate to the new path handling by replacing:

```go
tftypes.NewAttributePath().WithAttributeName("example")
```

With the equivalent:

```go
path.RootPath("example")
```

Then using the `(Path).At*` methods to extend the path definition instead of `(*tftypes.AttributePath).With*` methods:

| Current                  | New             |
| ------------------------ | --------------- |
| `WithAttributeName()`    | `AtName()`      |
| `WithElementKeyInt()`    | `AtListIndex()` |
| `WithElementKeyString()` | `AtMapKey()`    |
| `WithElementKeyValue()`  | `AtSetValue()`  |
@bflad bflad added breaking-change This PR introduces a breaking change or the resolution of this issue may require a breaking change. tech-debt Issues tracking technical debt that we're carrying. labels Jun 22, 2022
@bflad bflad added this to the v0.10.0 milestone Jun 22, 2022
@bflad bflad requested review from a team as code owners June 22, 2022 00:47
Copy link
Contributor

@bendbennett bendbennett left a comment

Choose a reason for hiding this comment

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

I've only looked through the path pkg, but that LGTM.

path/path.go Outdated
Comment on lines 113 to 129
// EmptyPath creates an empty attribute path. Provider code should use
// RootPath.
func EmptyPath() Path {
return Path{
steps: PathSteps{},
}
}

// RootPath creates a new attribute path starting with a root
// PathStepAttributeName.
func RootPath(rootAttributeName string) Path {
return Path{
steps: PathSteps{
PathStepAttributeName(rootAttributeName),
},
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given it worked out that this could be in its own package, I wonder if its worth shortening these these to just path.Root(string) and path.Empty()?

Copy link
Contributor

Choose a reason for hiding this comment

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

Big fan of short names.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll update and push up, one second

Copy link
Contributor

@detro detro left a comment

Choose a reason for hiding this comment

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

I like this a lot.

Especially the hierarchy and combination of Path and PathStep, I find it quite elegant.

The builder-patterns to compose Paths is also quite concise and expressive, leading to smaller lines of code.

The CI is failing, probably a place where the replacement is still needed, but feels easy to fix:

Run go test -v ./internal/framework5provider
# github.com/hashicorp/terraform-provider-corner/internal/framework5provider [github.com/hashicorp/terraform-provider-corner/internal/framework5provider.test]
Error: internal/framework5provider/resource_user.go:[1](https://github.com/hashicorp/terraform-plugin-framework/runs/6995717727?check_suite_focus=true#step:8:1)99:90: cannot use tftypes.NewAttributePath().WithAttributeName("name") (type *tftypes.AttributePath) as type path.Path in argument to tfsdk.ResourceImportStatePassthroughID
FAIL	github.com/hashicorp/terraform-provider-corner/internal/framework[5](https://github.com/hashicorp/terraform-plugin-framework/runs/6995717727?check_suite_focus=true#step:8:6)provider [build failed]
FAIL
Error: Process completed with exit code 2.

@bflad
Copy link
Contributor Author

bflad commented Jun 22, 2022

Updated the "starter" functions to just Empty and Root 👍 terraform-provider-corner will require a separate pull request to migrate everything for the breaking changes and verify things -- I'll work on that now and drop a link here.

@bflad
Copy link
Contributor Author

bflad commented Jun 22, 2022

Downstream terraform-provider-corner updates: hashicorp/terraform-provider-corner#73

@bflad
Copy link
Contributor Author

bflad commented Jun 22, 2022

If anyone has any additional feedback on these changes, please reach out to me and we can go through it together before the next release. I'll also ensure terraform-provider-corner is pointing at main after merge so CI keeps working.

@bflad bflad merged commit 02689b7 into main Jun 22, 2022
@bflad bflad deleted the bflad-attr-path branch June 22, 2022 17:24
bflad added a commit to hashicorp/terraform-provider-corner that referenced this pull request Jun 22, 2022
multani added a commit to multani/terraform-provider-incidentio that referenced this pull request Jul 19, 2022
multani added a commit to multani/terraform-provider-incidentio that referenced this pull request Jul 19, 2022
* Update module github.com/hashicorp/terraform-plugin-log to v0.6.0
* Update module github.com/hashicorp/terraform-plugin-go to v0.12.0
* Update module github.com/hashicorp/terraform-plugin-sdk/v2 to v2.19.0
* Update module github.com/hashicorp/terraform-plugin-framework to v0.10.0
* Fix for hashicorp/terraform-plugin-framework#390
* go mod tidy

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

breaking-change This PR introduces a breaking change or the resolution of this issue may require a breaking change. tech-debt Issues tracking technical debt that we're carrying.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants