-
Notifications
You must be signed in to change notification settings - Fork 37
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
Update SDKs #71
Update SDKs #71
Conversation
resolves #66 |
If there is going to be SDK updates, why not just go to the Terraform Plugin Framework instead? |
Rekicking build check |
I believe when these changes were created Hashcorp was still recommending SDKv2. |
Yes, that was my understanding. I was just wondering if it's worth still merging this, or move to the Terraform Plugin Framework instead. I figured if there was going to be effort to merge in a "refactor", might as well be the latest method, don't you think? I also think that something like this ( refactor ) would make more sense to be the |
5c6bee0
to
830c7bd
Compare
@CodeBleu I was looking into migrating directly to the terraform plugin framework and it will require a huge effort for that. It will be not as simple as migrating from v1 to v2. So my proposal is to continue the migration to sdkv2 to, at least, have a more recent version of the SDK, then start working on the migration to the plugin framework. I've already implemented some new things in my contribution, like muxing the provider and the terraform plugin testing in tests, so then it will be easier to migrate to the plugin framework. |
@vishesh92 @rohityadavcloud I've added another matrix to the acceptance test so multiple versions of terraform can be tested. |
@fabiomatavelli TF Plugin be a v0.7.0 milestone? |
@CodeBleu could be, but I'll let @rohityadavcloud decide on that. Also with muxing the migration can be partial, doing few resources/datasources on each release. |
.github/workflows/acceptance.yml
Outdated
terraform-version: | ||
- '0.12.*' | ||
- '0.13.*' | ||
- '0.14.*' | ||
- '0.15.*' | ||
- '1.0.*' | ||
- '1.1.*' | ||
- '1.2.*' | ||
- '1.3.*' | ||
- '1.4.*' | ||
- '1.5.*' | ||
- '1.6.*' | ||
- '1.7.*' |
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 it make sense to test all the versions of terraform or only the non EOL?
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.
only non EOL. I'd like to help/see more iterations pushed out vs. supporting EOL code.
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.
Ok, I've removed the <= 1.6
versions of Terraform then, based on https://endoflife.date/terraform.
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.
@fabiomatavelli what about Tofu versions?
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.
good catch @CodeBleu, I've added it now
06a4eb9
to
31bfc1a
Compare
@fabiomatavelli can you resolve the conflicts, thanks. I've approved the Github actions to start running tests, thanks for the PR. |
31bfc1a
to
d0825f3
Compare
@poddm @fabiomatavelli @rohityadavcloud What are we waiting on to get this updated? I was looking into testing some other stuff and noticed the version of the cloudstack-go version is really old in the terraform provider (2.13.2) and was trying to get that resolved locally for testing things and then remembered this was out here. |
I approved it awhile back. All of my open PRs are dependent on updating the cloudstack-go SDK. |
@poddm @rohityadavcloud what is the next step then? Can we merge it? It's all good to be merged so we can continue working on other PRs |
@fabiomatavelli @poddm I am running a little busy these days. Let me review this PR by next week. |
} | ||
|
||
func testSetValueOnResourceData(t *testing.T) { |
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.
Why were these tests removed?
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.
those functions were not in use @vishesh92
@fabiomatavelli I have reviewed the PR. Changes looks good. I have left comments where I have some questions about the changes. |
@vishesh92 issues fixed |
@vishesh92 can you merge this now? I feel like it should be a simple pressing of the merge button at this point, no? 😉 |
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.
LGTM, the build was a success
terraform-provider-cloudstack git:(feature/update-sdk) make build
==> Checking that code complies with gofmt requirements...
go install
go: downloading go1.21.6 (darwin/arm64)
go: downloading github.com/hashicorp/terraform-plugin-mux v0.15.0
go: downloading github.com/hashicorp/terraform-plugin-go v0.22.0
go: downloading github.com/apache/cloudstack-go/v2 v2.16.0
go: downloading github.com/hashicorp/terraform-plugin-sdk/v2 v2.33.0
go: downloading github.com/hashicorp/hcl/v2 v2.20.0
go: downloading github.com/zclconf/go-cty v1.14.3
go: downloading github.com/apparentlymart/go-textseg/v15 v15.0.0
go: downloading google.golang.org/grpc v1.62.0
go: downloading github.com/vmihailenco/msgpack/v5 v5.4.1
go: downloading github.com/hashicorp/terraform-registry-address v0.2.3
go: downloading google.golang.org/genproto/googleapis/rpc v0.0.0-20240123012728-ef4313101c80
was also able to deploy a vm via terraform and it was a success
Update of SDKs and Go version:
v2.13.2
tov2.16.0
The acceptance tests are still failing but the provider is compiling without any problem. All the necessary adjustments were made to make it work with the new terraform SDK.