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

Move all Go packages under internal/ #28723

Merged
merged 23 commits into from
May 17, 2021
Merged

Conversation

apparentlymart
Copy link
Member

@apparentlymart apparentlymart commented May 17, 2021

Aside from some historical exceptions under helper/ (which have since moved to the plugin SDK, all of the Go packages in this repository have always been for Terraform's internal use only, and not supported for importing by other codebases as if Terraform were a library.

However, this codebases predates the addition of Internal Packages in the Go toolchain, and so our packages were not positioned in such a way as to make it clear (using the usual Go idiom) that they are not suitable for external imports. Unfortunately this has meant that several times over the years we've refactored these internal packages and then heard from folks who mistakenly thought we were trying to create a public API surface because we'd broken their external system. That's a reasonable misunderstanding to make given how our repository is structured, but unfortunate that someone would find this out only after having invested time building something against an unsupported library API.

Although we've been using the internal directory for newly-added packages, we've long put off moving our existing packages under internal due to the disruption that would inevitably cause to existing code contributions that are not yet merged, and make it harder to backport commits to older releases.

However, we use tags in this repository to represent official Terraform releases and do so using a syntax that happens to match what the Go toolchain expects for semantic versioning of library modules. Due to that overlap, once we make a tag which looks like an initial semver release we will -- per normal Go idiom and Go toolchain assumptions -- unfortunately also be unintentionally communicating that this is a stable API surface that other Go modules can depend on.

Although there is never a good time for a change like this, it's time to bite the bullet and get this migration done so that we're communicating our intentions in the way the Go community expects, before we have a release tag in this repository that could be misinterpreted as a semantic versioning initial release and thus inadvertently encourage new dependents.

The only exception here is that I've not moved the github.com/hashicorp/terraform/version package to github.com/hashicorp/terraform/internal/version. Although we don't intend for that to be a public interface either, it's pragmatic to leave it where it is because our automatic release processes update the version numbers in that package and so leaving these alone avoids the need for the release process to handle two different possible paths to update, so that we can continue releasing against older major versions if we need to.

This sort of change is not practical to review in a diff-like form, so I can only promise that all I've done here is mechanically move directories around and rewrite import paths. There is no change to any code here aside from import statements, and so this new tree should be functionally equivalent to what came before it.

I'm intending to backport this to v0.15 too so that the current release branch is in the same directory layout as main in case we need to backport further changes.

This is part of a general effort to move all of Terraform's non-library
package surface under internal in order to reinforce that these are for
internal use within Terraform only.

If you were previously importing packages under this prefix into an
external codebase, you could pin to an earlier release tag as an interim
solution until you've make a plan to achieve the same functionality some
other way.
This is part of a general effort to move all of Terraform's non-library
package surface under internal in order to reinforce that these are for
internal use within Terraform only.

If you were previously importing packages under this prefix into an
external codebase, you could pin to an earlier release tag as an interim
solution until you've make a plan to achieve the same functionality some
other way.
This is part of a general effort to move all of Terraform's non-library
package surface under internal in order to reinforce that these are for
internal use within Terraform only.

If you were previously importing packages under this prefix into an
external codebase, you could pin to an earlier release tag as an interim
solution until you've make a plan to achieve the same functionality some
other way.
This is part of a general effort to move all of Terraform's non-library
package surface under internal in order to reinforce that these are for
internal use within Terraform only.

If you were previously importing packages under this prefix into an
external codebase, you could pin to an earlier release tag as an interim
solution until you've make a plan to achieve the same functionality some
other way.
This is part of a general effort to move all of Terraform's non-library
package surface under internal in order to reinforce that these are for
internal use within Terraform only.

If you were previously importing packages under this prefix into an
external codebase, you could pin to an earlier release tag as an interim
solution until you've make a plan to achieve the same functionality some
other way.
This is part of a general effort to move all of Terraform's non-library
package surface under internal in order to reinforce that these are for
internal use within Terraform only.

If you were previously importing packages under this prefix into an
external codebase, you could pin to an earlier release tag as an interim
solution until you've make a plan to achieve the same functionality some
other way.
This is part of a general effort to move all of Terraform's non-library
package surface under internal in order to reinforce that these are for
internal use within Terraform only.

If you were previously importing packages under this prefix into an
external codebase, you could pin to an earlier release tag as an interim
solution until you've make a plan to achieve the same functionality some
other way.
This is part of a general effort to move all of Terraform's non-library
package surface under internal in order to reinforce that these are for
internal use within Terraform only.

If you were previously importing packages under this prefix into an
external codebase, you could pin to an earlier release tag as an interim
solution until you've make a plan to achieve the same functionality some
other way.
This is part of a general effort to move all of Terraform's non-library
package surface under internal in order to reinforce that these are for
internal use within Terraform only.

If you were previously importing packages under this prefix into an
external codebase, you could pin to an earlier release tag as an interim
solution until you've make a plan to achieve the same functionality some
other way.
This is part of a general effort to move all of Terraform's non-library
package surface under internal in order to reinforce that these are for
internal use within Terraform only.

If you were previously importing packages under this prefix into an
external codebase, you could pin to an earlier release tag as an interim
solution until you've make a plan to achieve the same functionality some
other way.
This is part of a general effort to move all of Terraform's non-library
package surface under internal in order to reinforce that these are for
internal use within Terraform only.

If you were previously importing packages under this prefix into an
external codebase, you could pin to an earlier release tag as an interim
solution until you've make a plan to achieve the same functionality some
other way.
This is part of a general effort to move all of Terraform's non-library
package surface under internal in order to reinforce that these are for
internal use within Terraform only.

If you were previously importing packages under this prefix into an
external codebase, you could pin to an earlier release tag as an interim
solution until you've make a plan to achieve the same functionality some
other way.
This is part of a general effort to move all of Terraform's non-library
package surface under internal in order to reinforce that these are for
internal use within Terraform only.

If you were previously importing packages under this prefix into an
external codebase, you could pin to an earlier release tag as an interim
solution until you've make a plan to achieve the same functionality some
other way.
This is part of a general effort to move all of Terraform's non-library
package surface under internal in order to reinforce that these are for
internal use within Terraform only.

If you were previously importing packages under this prefix into an
external codebase, you could pin to an earlier release tag as an interim
solution until you've make a plan to achieve the same functionality some
other way.
This is part of a general effort to move all of Terraform's non-library
package surface under internal in order to reinforce that these are for
internal use within Terraform only.

If you were previously importing packages under this prefix into an
external codebase, you could pin to an earlier release tag as an interim
solution until you've make a plan to achieve the same functionality some
other way.
This is part of a general effort to move all of Terraform's non-library
package surface under internal in order to reinforce that these are for
internal use within Terraform only.

If you were previously importing packages under this prefix into an
external codebase, you could pin to an earlier release tag as an interim
solution until you've make a plan to achieve the same functionality some
other way.
This is part of a general effort to move all of Terraform's non-library
package surface under internal in order to reinforce that these are for
internal use within Terraform only.

If you were previously importing packages under this prefix into an
external codebase, you could pin to an earlier release tag as an interim
solution until you've make a plan to achieve the same functionality some
other way.
This is part of a general effort to move all of Terraform's non-library
package surface under internal in order to reinforce that these are for
internal use within Terraform only.

If you were previously importing packages under this prefix into an
external codebase, you could pin to an earlier release tag as an interim
solution until you've make a plan to achieve the same functionality some
other way.
This is part of a general effort to move all of Terraform's non-library
package surface under internal in order to reinforce that these are for
internal use within Terraform only.

If you were previously importing packages under this prefix into an
external codebase, you could pin to an earlier release tag as an interim
solution until you've make a plan to achieve the same functionality some
other way.
This is part of a general effort to move all of Terraform's non-library
package surface under internal in order to reinforce that these are for
internal use within Terraform only.

If you were previously importing packages under this prefix into an
external codebase, you could pin to an earlier release tag as an interim
solution until you've make a plan to achieve the same functionality some
other way.
This is part of a general effort to move all of Terraform's non-library
package surface under internal in order to reinforce that these are for
internal use within Terraform only.

If you were previously importing packages under this prefix into an
external codebase, you could pin to an earlier release tag as an interim
solution until you've make a plan to achieve the same functionality some
other way.
This is part of a general effort to move all of Terraform's non-library
package surface under internal in order to reinforce that these are for
internal use within Terraform only.

If you were previously importing packages under this prefix into an
external codebase, you could pin to an earlier release tag as an interim
solution until you've make a plan to achieve the same functionality some
other way.
@apparentlymart apparentlymart requested a review from a team May 17, 2021 19:58
@apparentlymart apparentlymart self-assigned this May 17, 2021
Previous commits have moved all of our Go packages under "internal", so
this is a retroactive update of non-Go references to those same packages.
@apparentlymart apparentlymart added the 0.15-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged label May 17, 2021
Copy link
Member

@jbardin jbardin left a comment

Choose a reason for hiding this comment

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

LGTM! 😉

Copy link
Contributor

@mildwonkey mildwonkey left a comment

Choose a reason for hiding this comment

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

WOOHOO!

@apparentlymart apparentlymart removed the 0.15-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged label May 17, 2021
@apparentlymart apparentlymart merged commit d35bc05 into main May 17, 2021
@apparentlymart apparentlymart deleted the f-internalize-packages branch May 17, 2021 21:09
@apparentlymart
Copy link
Member Author

Backported to v0.15.

codefromthecrypt pushed a commit to tetratelabs/func-e that referenced this pull request May 21, 2021
We formerly used historical conventions for project layout. After
summarizing earlier that the code here is only for use in CLI, we
started to move code under the internal package here
#222 (review)

This completes the work, only leaving "e2e" and "main.go" exposed. We
have some other thinking to do about "e2e", but for now it should be ok
to leave as a separate dir.

See https://dave.cheney.net/2019/10/06/use-internal-packages-to-reduce-your-public-api-surface
See hashicorp/terraform#28723
codefromthecrypt added a commit to tetratelabs/func-e that referenced this pull request May 21, 2021
We formerly used historical conventions for project layout. After
summarizing earlier that the code here is only for use in CLI, we
started to move code under the internal package here
#222 (review)

This completes the work, only leaving "e2e" and "main.go" exposed. We
have some other thinking to do about "e2e", but for now it should be ok
to leave as a separate dir.

See https://dave.cheney.net/2019/10/06/use-internal-packages-to-reduce-your-public-api-surface
See hashicorp/terraform#28723

Signed-off-by: Adrian Cole <adrian@tetrate.io>
Co-authored-by: Takeshi Yoneda <takeshi@tetrate.io>
@raphink
Copy link
Contributor

raphink commented Jun 16, 2021

@apparentlymart I totally understand the rationale behind this move and respect it.

In our Terraboard project, we rely on several of Terraform's "internal" APIs, in particular to parse Terraform State files (and its various formats over the years).

It makes sense for quite a few projects to be able to parse State files (and even Plans) to integrate with Terraform. Would it be possible for Hashicorp to provide a public API for this (whether or not it ends up being hosted in this repo or another), or do you consider these file formats to be strictly private APIs?

@alisdair
Copy link
Member

The state and plan file formats are not considered stable public interfaces. Instead, the JSON output from terraform show -json is the point of integration that we'd recommend. If there is missing information in the JSON representations, we'd be interested in hearing about that via a feature request issue.

@raphink
Copy link
Contributor

raphink commented Jun 22, 2021

Thanks for the reply @alisdair.

In Terraboard, we parse remote State files directly using Go, so having to run a command is very impractical. Would it be possible to provide an official public lib, even if it's outside of Terraform itself (maybe as a dep of Terraform)?

While we could parse the State files ourselves, the format is rather complex so we'll have to import the whole structure and parsing methods, + there's already been 5 different formats for these states, and the (now internal) Terraform lib provides an abstraction for this. For backwards compatibility, we want to support all previous versions of Terraform states so we can import them, but it's a bit counter-productive to copy this whole abstraction logic from internal Terraform lib.

What would you suggest we do?

@apparentlymart
Copy link
Member Author

Our only supported integration point is running Terraform CLI as a separate program. We don't have the resources to also maintain a separate stable library API.

Any approach using a separate library will end up just being a snapshot of the logic that was in Terraform CLI anyway, and not be able to respond automatically to Terraform CLI upgrades. If automatically supporting new versions of Terraform without changes to your system is not important to you then you can indeed feel free to copy the relevant parts of this codebase out into your own codebase. Of course, if you take that path then it'll be up to you to decide when to update your copy if Terraform's own behavior changes in a newer version.

An alternative is to follow a similar principle that Terraform Cloud does: after each successful run it runs terraform show -json to capture a snapshot of the plan or state (depending on what sort of operation it just ran) and saves that snapshot so it'll be available for other downstream features, such as Terraform Cloud's own view of a plan or state, to parse and render. In other words, it's the responsibility of whatever automation is running Terraform to capture the resulting artifacts and upload them to whatever other consumers need it, rather than having those consumers request the data directly from Terraform CLI.

If you'd like to discuss this further, I'd encourage you to open a topic in the community forum. There we can discuss more details about the two strategies above, although as noted above there is no path to Terraform having a Go library interface in addition to its CLI interface for the foreseeable future, because we need to keep our focus on the CLI tool.

@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, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants