cloudstack-terraform-provider icon indicating copy to clipboard operation
cloudstack-terraform-provider copied to clipboard

feat: migrate to terraform plugin framework

Open fabiomatavelli opened this issue 1 year ago • 1 comments

This PR will:

  • Create the plugin framework provider
  • Migrate the service offering resource to the plugin framework

fabiomatavelli avatar Apr 20 '24 18:04 fabiomatavelli

@CodeBleu @vishesh92 I started drafting the migration to the terraform plugin framework, base on the #71 , to see the effort to migrate the provider and resources. For that I've migrated the service offering resource. You can see the diff here.

I'm open to feedbacks on it.

fabiomatavelli avatar Apr 20 '24 18:04 fabiomatavelli

@fabiomatavelli Just wondering the status of this? Is this dependent on the following being merged?

https://github.com/apache/cloudstack-terraform-provider/pull/71

CodeBleu avatar Jun 07 '24 20:06 CodeBleu

This is ready to be merged @CodeBleu , I just don't have the permission to merge it

fabiomatavelli avatar Jun 07 '24 20:06 fabiomatavelli

This is ready to be merged @CodeBleu , I just don't have the permission to merge it

I just still see it in "Draft" mode.

CodeBleu avatar Jun 07 '24 20:06 CodeBleu

Ah, sorry @CodeBleu , my bad, this is indeed waiting for #71

fabiomatavelli avatar Jun 07 '24 21:06 fabiomatavelli

@fabiomatavelli now that #71 is merged (🥳 ), I'm hoping that removing this from Draft mode, will be able to be merged, since @kiranchavala approved it.

CodeBleu avatar Jun 21 '24 12:06 CodeBleu

@CodeBleu I've removed the draft from it, but I think I'll need to rebase it

fabiomatavelli avatar Jun 21 '24 13:06 fabiomatavelli

@kiranchavala if you can approve the CIs to run again 🙏

fabiomatavelli avatar Jun 21 '24 16:06 fabiomatavelli

@fabiomatavelli I have approved the github actions. Acceptance tests seems to be failing because of the below error:

 === RUN   TestAccServiceOfferingDataSource_basic
    data_source_cloudstack_service_offering_test.go:32: Step 1/1 error: Error running pre-apply plan: exit status 1
        
        Error: Invalid resource type
        
          on terraform_plugin_test.tf line 12, in resource "cloudstack_service_offering" "service-offering-resource":
          12: resource "cloudstack_service_offering" "service-offering-resource" {
        
        The provider hashicorp/cloudstack does not support resource type
        "cloudstack_service_offering".
        
        Did you intend to use the data source "cloudstack_service_offering"? If so,
        declare this using a "data" block instead of a "resource" block.
--- FAIL: TestAccServiceOfferingDataSource_basic (0.14s)

vishesh92 avatar Jun 21 '24 19:06 vishesh92

@vishesh92 @kiranchavala can you please review it again and re-approve the CI to run 🙏

I had to move the V6 (terraform plugin framework) to be tested with a muxed provider instead, as we still need to call resources/datasources in the V5 provider (terraform provider sdk).

fabiomatavelli avatar Jun 23 '24 13:06 fabiomatavelli

@vishesh92 @kiranchavala Is this something that can be merged now? I feel like we were making good progress to get this provider over the hump with the new release of 0.5.0, and am hoping we can keep that momentum going and keep updating this project more frequently. With the #71 being merged and hopefully this one soon, I think this is really good for moving forward. There are some other PR's out there I think would be good to get merged as well, and hopefully we can get a new 0.6.0 release soon?

CodeBleu avatar Jun 30 '24 01:06 CodeBleu

@fabiomatavelli @CodeBleu since we recently had a TF release, the next one we can target for Q3/Q4 this year. If you're blocked then let us know, otherwise due to our $dayjobs and other community work (the release work for example), our engagement and responses may be delayed.

rohityadavcloud avatar Jul 02 '24 08:07 rohityadavcloud

@vishesh92 @rohityadavcloud can this one be merged also?

fabiomatavelli avatar Jul 02 '24 12:07 fabiomatavelli

@fabiomatavelli There are changes around service offering in PR #77 by @poddm. Does this PR supersede the changes in PR #77?

vishesh92 avatar Jul 02 '24 17:07 vishesh92

@vishesh92 yes, this will be the first resource updated to the tf framework. Do you think the approach used in the #77 is better, by splitting into different resources?

This one is just updating to the tf framework and adding all the available params for the service offering.

fabiomatavelli avatar Jul 02 '24 17:07 fabiomatavelli

@vishesh92 yes, this will be the first resource updated to the tf framework. Do you think the approach used in the #77 is better, by splitting into different resources?

This one is just updating to the tf framework and adding all the available params for the service offering.

I am not using this provider. So, this is just my opinion. Splitting into different resources might make it easier for the user to define the service offerings using terraform. But I am not sure how easy it would be for users to migrate to the newer version. And if it would cause issues any issue with service offering datasource.

vishesh92 avatar Jul 02 '24 18:07 vishesh92

@fabiomatavelli can you just remove the offering stuff from this PR, so we can get this plugin framework across the line, and then address the options of splitting up the offering resources or not later?

CodeBleu avatar Jul 03 '24 00:07 CodeBleu

@CodeBleu done

fabiomatavelli avatar Jul 04 '24 11:07 fabiomatavelli

@vishesh92 @kiranchavala Can this be merged now?

CodeBleu avatar Jul 04 '24 12:07 CodeBleu

@fabiomatavelli @CodeBleu since we recently had a TF release, the next one we can target for Q3/Q4 this year. If you're blocked then let us know, otherwise due to our $dayjobs and other community work (the release work for example), our engagement and responses may be delayed.

CC @kiranchavala @vishesh92 @rohityadavcloud

With the release of Cloudstack 4.19.1 now out, I'm hoping we can get this PR across the line. I feel like this should be just a matter of clicking the "merge" button, as there have been a lot of back and forth on this one and the last concern about it, had that piece removed to make it easier to proceed with merging this.

I think this PR for the new plugin framework is a pretty important one to get merged in, so that efforts can be made to start porting all of the data and resources over to the new framework.

I appreciate all you are doing for the community and know you're busy, but thought it was worth getting this one merged.

Thanks!

CodeBleu avatar Jul 24 '24 18:07 CodeBleu

Fine by me, but since I'm less of a developer these days - let's wait from @vishesh92 and/or @kiranchavala to advise.

rohityadavcloud avatar Jul 25 '24 04:07 rohityadavcloud

Thanks!

CodeBleu avatar Jul 25 '24 04:07 CodeBleu