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

feat: integrate terraform-plugin-mux and reimplement r/network_pool using the plugin framework #192

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

spacegospod
Copy link
Contributor

Summary of Pull Request

Overview

Terraform offers 2 versions of its SDK

  1. github.com/hashicorp/terraform-plugin-sdk/v2 (legacy)
  2. github.com/hashicorp/terraform-plugin-framework (latest)

The latter offers numerous benefits over its predecessor and while migrating to the latest SDK is not mandatory it is certainly beneficial.

Instead of migrating the entire provider in a single change I am utilizing the plugin mux which allows for 2 implementations of a provider, one for each protocol version, to coexist in the same binary.
RPC calls for resources will automatically be routed to the implementation that hosts them which allows us to migrate the provider to the Plugin Framework one resource at a time.

List of changes

Dependencies

Added dependencies to github.com/hashicorp/terraform-plugin-framework, github.com/hashicorp/terraform-plugin-mux and their subsidiaries

Resources

This change only affects resource_network_pool. All other resources and data sources remain unchanged.
The schema for resource_network_pool remains identical to the original and no breaking changes are expected.

Tests

No changes to the tests cases are necessary. The migrated resource is expected to be a carbon copy of the original in terms of functionality.
The only modification I've made is to bootstrap the correct provider implementation for the affected tests.

Type of Pull Request

  • This is a bug fix.
  • This is an enhancement or feature.
  • This is a code style/formatting update.
  • This is a documentation update.
  • This is a refactoring update.
  • This is a chore update
  • This is something else.
    Please describe:

Related to Existing Issues

Issue Number: N/A

Test and Documentation Coverage

Ran TestAccResourceVcfNetworkPool to verify that the migrated resource is identical to the original
Ran TestAccResourceVcfCeip to verify that non-migrated resources are not broken by the changes to the provider configuration
Ran the provider binary separately to verify that the actual deliverable works as before

For bug fixes or features:

  • Tests have been completed.
  • Documentation has been added/updated.

Breaking Changes?

  • Yes, there are breaking changes.
  • No, there are no breaking changes.

@spacegospod spacegospod self-assigned this Jun 20, 2024
@github-actions github-actions bot added documentation Documentation dependencies Dependencies chore Chore needs-review Needs Review provider labels Jun 20, 2024
@spacegospod spacegospod changed the title Integrate terraform-plugin-mux and upgrade resource_network_pool to protocol v6 Integrate terraform-plugin-mux and reimplement resource_network_pool using the Plugin Framework Jun 20, 2024
res.Diagnostics.Append(diag.NewErrorDiagnostic("Failed to connect to the SDDC Manager", err.Error()))
}

p.SddcManagerClient = client
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: This is such a cringe language where they make you use single letter variables. I purposefully avoided (p *FrameworkProvider) and would do (provider *FrameworkProvider)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be fair the language doesn't enforce this, it's just that most examples, even in effective go, are like that.

In this particular case calling the variable provider would shadow the package name which is a bigger problem.
We could go with prov, frameworkProvider, providerRef but I'd also sleep well at night if I leave it at p

Copy link
Contributor

Choose a reason for hiding this comment

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

There is no :cringe: emoji...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

renamed the reference to frameworkProvider

For the record - I still disagree with this but it's more important to get this change going

@tenthirtyam
Copy link
Contributor

tenthirtyam commented Jun 20, 2024

Will review by end of week.

dimitarproynov
dimitarproynov previously approved these changes Jul 1, 2024
}
}

func getAttributeValue[T string | bool](data T, envVar string) interface{} {
Copy link
Contributor

Choose a reason for hiding this comment

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

I have the feeling you're going to use a lot of methods like these in this new version of the provider. Maybe you should extract them into a separate utils file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Data access is actually quite decent with the framework. This method was necessary because I wanted to retain the provider's capability to read its settings from environment variables (the standard library can only read them as strings).

Do you mind if we wait until we migrate at least one more resource? It may turn out that we only need this in this one place.

internal/provider/resource_network_pool.go Outdated Show resolved Hide resolved
res.Diagnostics.Append(diags...)

ctx, cancel := context.WithTimeout(ctx, timeout)
defer cancel()
Copy link
Contributor

Choose a reason for hiding this comment

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

Where does this function come from? It cancels the above context?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for mentioning this, now I noticed that this timeout doesn't actually work.

But first to answer your question - https://developer.hashicorp.com/terraform/plugin/framework/resources/timeouts
This is how you declare a default timeout in the plugin framework. The purpose of this block is to mimic the original implementation where there's a default 30 min timeout on creating network pools.
The cancel function terminates the execution with the same error that we get today - context deadline exceeded.

This code isn't where it should be though, I'm going to move it up to the place where I create the request parameters. This timed context should be used so that the actual request has a time limit.

internal/provider/resource_network_pool.go Show resolved Hide resolved
@tenthirtyam tenthirtyam changed the title Integrate terraform-plugin-mux and reimplement resource_network_pool using the Plugin Framework feat: integrate terraform-plugin-mux and reimplement r/network_pool using the plugin framework Jul 5, 2024
…_pool using the plugin framework

Signed-off-by: Stoyan Zhelyazkov <[email protected]>
@tenthirtyam tenthirtyam changed the title feat: integrate terraform-plugin-mux and reimplement r/network_pool using the plugin framework feat: integrate terraform-plugin-mux and reimplement r/network_pool using the plugin framework Jul 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Chore dependencies Dependencies documentation Documentation needs-review Needs Review provider
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants