-
Notifications
You must be signed in to change notification settings - Fork 45
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
feat: expose the FrameworkProvider type and functions via equinix/provider #735
base: main
Are you sure you want to change the base?
Conversation
…ameworkProvider Signed-off-by: Marques Johansson <[email protected]>
Signed-off-by: Marques Johansson <[email protected]>
Signed-off-by: Marques Johansson <[email protected]>
Signed-off-by: Marques Johansson <[email protected]>
b247e11
to
ff3f466
Compare
IMO keeping this private--and making the SDK provider private too--and requiring the downstreams to do more work is preferable to establishing an expectation that this provider can be used as a Go module and that we are going to provide a stable public interface. I believe this aligns with how HashiCorp-managed providers (AWS, Google, Azure) are structured--and probably others--although I don't know which or how many of those providers are used as the basis for downstream integrations. |
The public interface would be restricted to the Provider/FrameworkProvider types and their constructors. The implementation details remain private. The public interfaces of this provider/ package would be tied to the upstream FrameworkProvider changes, which could change. The SDK Provider interface stayed consistent for many years with new interfaces being exposed as new packages (SDKv2).
We are in v2+ currently and haven't adopted v2/ paths. We have many public functions in equinix/ that we intend to move to internal. We don't have a strong argument around this today, IMHO.
There are other potential motives for this as Terraform providers became desirable as surrogates for Go CRUD implementations, translations of APIs to declarative syntax. :tinfoil_hat: |
moving equinix/provider.go's |
These 2 comments seem to be in direct conflict:
If the provider can be importable and not follow backwards compatibility guarantees, then it seems like it should be fine to move the SDK provider. I maintain my concerns about introducing the potential overhead of ensuring that provider releases don't break arbitrary downstream tools; the existing downstream maintenance burden is a clear indicator of where support responsibilities lie. That said, it sounds like we're taking it as a given that the provider should be importable, and if that's the case I'll reiterate my suggestion from #619 (comment):
|
@@ -1,6 +1,6 @@ | |||
module github.com/equinix/terraform-provider-equinix | |||
|
|||
go 1.22 | |||
go 1.22.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't specify patch versions in other Go things (whether shipped as modules or binaries). What makes it necessary here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When tools try to fetch the toolchain for this project they find 1.22
, which is not a release version -- there is no toolchain defined without full semver.
$ go version
go: downloading go1.22 (linux/amd64)
go: download go1.22 for linux/amd64: toolchain not available
- cmd/go: mod tidy reports toolchain not available with 'go 1.21' golang/go#62278 (comment)
- https://go.dev/doc/toolchain
An alternative approach is to specify toolchain 1.22.0
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That said, I'm not sure what about my environment specifically triggered this. It may be a difference in how Go is installed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does that error happen for you in metal-cli
and equinix-sdk-go
as well, or only here? And it's preventing you from running go mod tidy
or you're trying to do something else? That doesn't seem to be impacting me or Renovate, but I'm not clear exactly how to reproduce this problem.
Signed-off-by: Marques Johansson <[email protected]>
In development of Terraform dependent providers, such as Crossplane and Pulumi, access to the Terraform Framework Provider is necessary in order to access the metadata and implementation of this provider directly.
The Equinix (SDK) Provider is currently exposed in a similar way, as equinix/provider.go (equinix.Provider). This PR exposes equinix/provider/framework.go (moving it from internal/provider/provider.go) as
equinix.provider.FrameworkProvider
. (Similiar PR on the Linode TF Provider)This branch can be used directly in the go.mod of Crossplane Provider PRs to test that this minor Terraform refactor has impact there.
I would have moved equinix/provider.go's
Provider
and related tests to equinix/provider/sdkv2.go and sdkv2_test.go, but that change would have been more substantial. This would be simpler to apply after #622 and perhaps after a similar internal/ PR for Metal where the remaining private types and functions from provider.go and provider_test.go in the equinix package are no longer needed.Alternative approaches to take advantage of the implementation involve copying the Terraform provider code into those projects. This creates downstream build and sync work that can be easily avoided by exposing the FrameworkProvider.
Another alternative approach, taken by some downstream tools, is to fetch the Terraform provider binary, run the schema export command, and then process that JSON through conversion libraries specific to that downstream tool. This approach is also burdensome for downstream tools and maintainers.
Additional minor changes introduced in this PR:
Closes #619