Skip to content

Commit

Permalink
terraform: Add deprecation Go documentation comments (#171)
Browse files Browse the repository at this point in the history
Reference: #165

Except in certain known provider testing use cases without replacement (yet), this deprecates various `terraform` package functionality. The `terraform` package contains legacy Terraform core logic which has been copied to terraform-plugin-sdk and terraform-plugin-testing over the years and continually exported due to the complexity of rewriting developer functionality using the machine-readable interfaces for Terraform, such as JSON state. Much of this now-deprecated `terraform` package logic should have been omitted when this Go module was created, but that step was missed, so it is left as-is with deprecation notices following the Go module versioning guidelines. In reality, the entire `terraform` package will be removed in a future major version, however this change pragmatically leaves certain functionality without deprecation notices for now until  replacement functionality is available so developers will not need to silence linting tools without it being actionable yet.

This change opts to not include a changelog entry here because it could be more confusing for developers, who might feel like they need to go check for usage when it likely affects only a very small (if any) portion of the population. Their usage externally would be unexpected as all of these are unintentionally part of the exported Go API having been just copied Terraform core "internal" code from some historical time and it is not likely any future changes will occur to their logic since they fall outside the intention of the Go module being acceptance testing of Terraform Providers. If/when it becomes time to deprecate the `terraform.State` type, which is externally prevalent due to `helper/resource.TestCheckFunc`, that certainly warrants a changelog entry (and maybe even website documentation and upgrade tooling) with its replacement. This change attempted to be careful and leave other somewhat prevalent functionality under the `TestCheckFunc` expected use case, such as `(terraform.State).RootModule()`, also without deprecation comments for now since there is no replacement yet. If there are other similar parts of this now-deprecated functionality which adversely affect provider acceptance testing use cases, the deprecation comments can be removed until replacement functionality is in place. To that end, if there is a valid use case for this now-deprecated functionality, developers should create a GitHub issue for tracking.
  • Loading branch information
bflad committed Aug 29, 2023
1 parent c132058 commit 5eb089d
Show file tree
Hide file tree
Showing 11 changed files with 283 additions and 73 deletions.
4 changes: 2 additions & 2 deletions helper/resource/state_shim.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ type shimmedState struct {
}

func shimStateFromJson(jsonState *tfjson.State) (*terraform.State, error) {
state := terraform.NewState()
state := terraform.NewState() //nolint:staticcheck // legacy usage
state.TFVersion = jsonState.TerraformVersion

if jsonState.Values == nil {
Expand Down Expand Up @@ -124,7 +124,7 @@ func (ss *shimmedState) shimStateModule(sm *tfjson.StateModule) error {
}
}

mod := ss.state.AddModule(path)
mod := ss.state.AddModule(path) //nolint:staticcheck // legacy usage
for _, res := range sm.Resources {
resourceState, err := shimResourceState(res)
if err != nil {
Expand Down
8 changes: 4 additions & 4 deletions helper/resource/testing.go
Original file line number Diff line number Diff line change
Expand Up @@ -1610,7 +1610,7 @@ func modulePrimaryInstanceState(ms *terraform.ModuleState, name string) (*terraf
// modulePathPrimaryInstanceState returns the primary instance state for the
// given resource name in a given module path.
func modulePathPrimaryInstanceState(s *terraform.State, mp addrs.ModuleInstance, name string) (*terraform.InstanceState, error) {
ms := s.ModuleByPath(mp)
ms := s.ModuleByPath(mp) //nolint:staticcheck // legacy usage
if ms == nil {
return nil, fmt.Errorf("No module found at: %s", mp)
}
Expand All @@ -1621,7 +1621,7 @@ func modulePathPrimaryInstanceState(s *terraform.State, mp addrs.ModuleInstance,
// primaryInstanceState returns the primary instance state for the given
// resource name in the root module.
func primaryInstanceState(s *terraform.State, name string) (*terraform.InstanceState, error) {
ms := s.RootModule()
ms := s.RootModule() //nolint:staticcheck // legacy usage
return modulePrimaryInstanceState(ms, name)
}

Expand All @@ -1640,7 +1640,7 @@ func indexesIntoTypeSet(key string) bool {
func checkIfIndexesIntoTypeSet(key string, f TestCheckFunc) TestCheckFunc {
return func(s *terraform.State) error {
err := f(s)
if err != nil && s.IsBinaryDrivenTest && indexesIntoTypeSet(key) {
if err != nil && indexesIntoTypeSet(key) {
return fmt.Errorf("Error in test check: %s\nTest check address %q likely indexes into TypeSet\nThis is currently not possible in the SDK", err, key)
}
return err
Expand All @@ -1650,7 +1650,7 @@ func checkIfIndexesIntoTypeSet(key string, f TestCheckFunc) TestCheckFunc {
func checkIfIndexesIntoTypeSetPair(keyFirst, keySecond string, f TestCheckFunc) TestCheckFunc {
return func(s *terraform.State) error {
err := f(s)
if err != nil && s.IsBinaryDrivenTest && (indexesIntoTypeSet(keyFirst) || indexesIntoTypeSet(keySecond)) {
if err != nil && (indexesIntoTypeSet(keyFirst) || indexesIntoTypeSet(keySecond)) {
return fmt.Errorf("Error in test check: %s\nTest check address %q or %q likely indexes into TypeSet\nThis is currently not possible in the SDK", err, keyFirst, keySecond)
}
return err
Expand Down
4 changes: 2 additions & 2 deletions helper/resource/testing_new.go
Original file line number Diff line number Diff line change
Expand Up @@ -356,7 +356,7 @@ func getState(ctx context.Context, t testing.T, wd *plugintest.WorkingDir) (*ter
}

func stateIsEmpty(state *terraform.State) bool {
return state.Empty() || !state.HasResources()
return state.Empty() || !state.HasResources() //nolint:staticcheck // legacy usage
}

func planIsEmpty(plan *tfjson.Plan) bool {
Expand All @@ -375,7 +375,7 @@ func testIDRefresh(ctx context.Context, t testing.T, c TestCase, wd *plugintest.

// Build the state. The state is just the resource with an ID. There
// are no attributes. We only set what is needed to perform a refresh.
state := terraform.NewState()
state := terraform.NewState() //nolint:staticcheck // legacy usage
state.RootModule().Resources = make(map[string]*terraform.ResourceState)
state.RootModule().Resources[c.IDRefreshName] = &terraform.ResourceState{}

Expand Down
2 changes: 1 addition & 1 deletion helper/resource/testing_new_config.go
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,6 @@ func testStepNewConfig(ctx context.Context, t testing.T, c TestCase, wd *plugint
if step.Check != nil {
logging.HelperResourceTrace(ctx, "Using TestStep Check")

state.IsBinaryDrivenTest = true
if step.Destroy {
if err := step.Check(stateBeforeApplication); err != nil {
return fmt.Errorf("Check failed: %w", err)
Expand Down Expand Up @@ -244,6 +243,7 @@ func testStepNewConfig(ctx context.Context, t testing.T, c TestCase, wd *plugint
return err
}

//nolint:staticcheck // legacy usage
if state.Empty() {
return nil
}
Expand Down
2 changes: 1 addition & 1 deletion helper/resource/testing_new_import_state.go
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ func testStepNewImportState(ctx context.Context, t testing.T, helper *plugintest
continue
}

is := r.Primary.DeepCopy()
is := r.Primary.DeepCopy() //nolint:staticcheck // legacy usage
is.Ephemeral.Type = r.Type // otherwise the check function cannot see the type
states = append(states, is)
}
Expand Down
Loading

0 comments on commit 5eb089d

Please sign in to comment.