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

terraform: Add deprecation Go documentation comments #171

Merged
merged 2 commits into from
Aug 29, 2023

Conversation

bflad
Copy link
Contributor

@bflad bflad commented Aug 28, 2023

Closes #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.

If there is a valid use case for this functionality, developers should create a GitHub issue for tracking.

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.

If there is a valid use case for this functionality, developers should create a GitHub issue for tracking.
@bflad bflad requested a review from a team as a code owner August 28, 2023 17:58
Copy link
Member

@austinvalle austinvalle left a comment

Choose a reason for hiding this comment

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

lgtm 🚀

Do we typically add changelogs for adding deprecation notices? Or just wait until the actual deprecation occurs

@bflad
Copy link
Contributor Author

bflad commented Aug 28, 2023

Do we typically add changelogs for adding deprecation notices?

Sorry I should've mentioned that in the commit message.

The short answer is yes for intentionally designed Go packages, however I'm opting to not include a changelog entry here because I think it'll 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 we likely would not support any changes to their logic since they fall outside the intention of the Go module being acceptance testing of Terraform Providers. If/when we do deprecate something like the terraform.State type, which is externally prevalent thanks to helper/resource.TestCheckFunc, that definitely warrants a changelog entry (and maybe even website documentation and upgrade tooling) with its replacement. I tried 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. We can undo any "oops this is regularly called by providers implementing their own TestCheckFunc" by removing the deprecation comments.

We can certainly add a changelog entry in this case if folks disagree with my sentiment though. Happy to discuss more.

@austinvalle
Copy link
Member

austinvalle commented Aug 28, 2023

however I'm opting to not include a changelog entry here because I think it'll be more confusing for developers

This makes sense to me; changelog should be geared for consumers of the module and if a note would bring confusion, omitting sounds like a good call. Thanks for the context 👍🏻

@bflad bflad added this to the v1.5.0 milestone Aug 29, 2023
@bflad bflad merged commit 5eb089d into main Aug 29, 2023
5 checks passed
@bflad bflad deleted the bflad/terraform-package-deprecations branch August 29, 2023 13:15
@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 Sep 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deprecate Unreferenced terraform Package Functionality
2 participants