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

Update SDKs #71

Merged
merged 33 commits into from
Jun 21, 2024
Merged

Update SDKs #71

merged 33 commits into from
Jun 21, 2024

Conversation

fabiomatavelli
Copy link
Collaborator

@fabiomatavelli fabiomatavelli commented Aug 19, 2023

Update of SDKs and Go version:

  • Go v1.21
  • Terraform SDK from v1 to v2
  • Cloudstack SDK from v2.13.2 to v2.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.

@poddm
Copy link
Contributor

poddm commented Nov 28, 2023

resolves #66

@rohityadavcloud rohityadavcloud added this to the v0.5.0 milestone Jan 30, 2024
@CodeBleu
Copy link
Collaborator

If there is going to be SDK updates, why not just go to the Terraform Plugin Framework instead?

@rohityadavcloud
Copy link
Member

Rekicking build check

@poddm
Copy link
Contributor

poddm commented Feb 29, 2024

If there is going to be SDK updates, why not just go to the Terraform Plugin Framework instead?

I believe when these changes were created Hashcorp was still recommending SDKv2.

@CodeBleu
Copy link
Collaborator

CodeBleu commented Feb 29, 2024

If there is going to be SDK updates, why not just go to the Terraform Plugin Framework instead?

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 v0.5.0, but for now it would be nice to get a v0.4.x out with the latest updates that are already commited.

@vishesh92 vishesh92 closed this Mar 1, 2024
@vishesh92 vishesh92 reopened this Mar 1, 2024
@fabiomatavelli
Copy link
Collaborator Author

Indeed, I haven't seen the Terraform Plugin Framework at the time I did it. If you think it is better to go straight to it, then I can close this one @CodeBleu @poddm as I'm not sure I'll have time to work on it.

@fabiomatavelli
Copy link
Collaborator Author

@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.

@fabiomatavelli
Copy link
Collaborator Author

@vishesh92 @rohityadavcloud I've added another matrix to the acceptance test so multiple versions of terraform can be tested.

@CodeBleu
Copy link
Collaborator

@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.

@fabiomatavelli TF Plugin be a v0.7.0 milestone?

@fabiomatavelli
Copy link
Collaborator Author

fabiomatavelli commented Mar 16, 2024

@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.

Comment on lines 99 to 114
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.*'
Copy link
Collaborator Author

@fabiomatavelli fabiomatavelli Mar 16, 2024

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?

Copy link
Contributor

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.

Copy link
Collaborator Author

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.

Copy link
Collaborator

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?

Copy link
Collaborator Author

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

@rohityadavcloud
Copy link
Member

@fabiomatavelli can you resolve the conflicts, thanks. I've approved the Github actions to start running tests, thanks for the PR.

@CodeBleu
Copy link
Collaborator

CodeBleu commented Jun 7, 2024

@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.

image

image

@poddm
Copy link
Contributor

poddm commented Jun 7, 2024

@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.

image

image

I approved it awhile back. All of my open PRs are dependent on updating the cloudstack-go SDK.

@fabiomatavelli
Copy link
Collaborator Author

@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

@vishesh92
Copy link
Member

@fabiomatavelli @poddm I am running a little busy these days. Let me review this PR by next week.

}

func testSetValueOnResourceData(t *testing.T) {
Copy link
Member

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?

Copy link
Collaborator Author

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

@vishesh92
Copy link
Member

@fabiomatavelli I have reviewed the PR. Changes looks good. I have left comments where I have some questions about the changes.
Apart from this, there are sinor code style issues related to indentation at some places which is due to tabs and spaces.

@fabiomatavelli
Copy link
Collaborator Author

@vishesh92 issues fixed

@CodeBleu
Copy link
Collaborator

@vishesh92 can you merge this now? I feel like it should be a simple pressing of the merge button at this point, no? 😉

Copy link
Collaborator

@kiranchavala kiranchavala left a 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

@kiranchavala kiranchavala merged commit 05dc88b into apache:main Jun 21, 2024
9 checks passed
@fabiomatavelli fabiomatavelli deleted the feature/update-sdk branch June 21, 2024 16:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants