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

Update to sdk v2

Open tete17 opened this issue 4 months ago • 4 comments

Most of the heavy lifting is done by the tool and I just had to change the signature of some lambdas to include the newly added context.

Som deprecations have been added as a result of this but we can be dealt with it other MR.

This drops support for terraform <= 0.11.

My testing capabilities are very limited as my company owns an cloudamqp account but I personally don't. I have checked through the update documentation and nothing pops up that we should communicate to users other than the old terraform version support.

Closes #140

tete17 avatar Mar 02 '24 13:03 tete17

Nice! What is the tool you used?

This will be an excellent test for https://github.com/cloudamqp/terraform-provider-cloudamqp/pull/257 once finished

dentarg avatar Mar 02 '24 14:03 dentarg

Hey @dentarg is the plan of #257 to make use of the terraform provided testing harness?

This update includes a testing ground we may want to look at to avoid creating a testing system only to migrate to the "official" one

tete17 avatar Mar 09 '24 00:03 tete17

@tete17 we are using Terraforms Acceptance test together with go-vcr. Note that each test require some tweaking. With Go VCR we can record requests/responses to the API backend into fixtures, then do playbacks of previous tests. This significantly speed up the test times to ms instead of minutes for each test.

tbroden84 avatar Mar 11 '24 06:03 tbroden84

Note about merged https://github.com/cloudamqp/terraform-provider-cloudamqp/pull/257 and our changes to how testing is done.

From the conflict list, the biggest impact should be the test files.

  • Removed all data source tests (some are tested within the resource tests)
  • Restructured of affected resource tests

Minor impact

  • New dependencies (go.mod/go.sum)
  • Updated provider_test.go - missing the v2 changes
  • provider.go - missing the v2 changes

tbroden84 avatar May 02 '24 13:05 tbroden84

Have a rebased version of this branch locally. However some issues with the new tests. Now with SDK v2, terraform-json dependency require a newer version. Found some strange mismatches in the recorded fixtures. Each test also have a huge increase in running time. Will continue to investigate this.

tbroden84 avatar May 02 '24 14:05 tbroden84

Hey @tbroden84 give me a few hours I am on it :)

tete17 avatar May 02 '24 15:05 tete17

The branch should be updated

@tbroden84 I am unable to run the test on my own but happy to step in and help with the fixtures if you share them somehow

tete17 avatar May 02 '24 16:05 tete17

What's the problem with running the tests? The fixtures should be in the repo already

dentarg avatar May 02 '24 17:05 dentarg

Dont i need a cloudamqp token?

The documentation says something about loading a .enc file.

What is the colmand?

tete17 avatar May 02 '24 18:05 tete17

Should not be need. The README has instructions.

dentarg avatar May 02 '24 18:05 dentarg

Just to mention, still need some tweaking for the test to work for SDK v2. Noticed all tests claimed there was missing interactions, which are there. There is also a deprecated function on how to chose the provider for each test case and some overall changes to be overlooked before they can be used. So it was not as straight forward that I had hoped.

tbroden84 avatar May 02 '24 18:05 tbroden84

Silly me now I am running the acceptance test locally.

I am facing the missing interactions you guys mention. It seems the provider is being configured multiple times but I have no idea why.

@tbroden84 what deprecated function to select the provider does it still use I can't find it.

tete17 avatar May 02 '24 19:05 tete17

It seems to be calling the ValidateChange("plan") function for an instance more times than before.

Not sure why but it would be interesting to record the fixtures again @tbroden84 @dentarg

tete17 avatar May 02 '24 19:05 tete17

In fact if you remove the diff functions that execute the api calls the test work.

I am also now seeing the long delay you mention. It seems to be happening right at the end of the test after all the api calls have been completed.

tete17 avatar May 02 '24 19:05 tete17

Further news about the delay. Running the test withTF_ACC_LOG=TRACE & TF_LOG=TRACE shows the reason for most of the delays.

There is a lot of artificial waits from the https://github.com/84codes/go-api package like these ones:

  • https://github.com/84codes/go-api/blob/f221038cde0073163b4107860971cd459e136a8d/api/instance.go#L123
  • https://github.com/84codes/go-api/blob/main/api/alarms.go#L120

This is causing all test to last at least 10 seconds if not more .

I think they only last that long because terraform has slightly changed the order on which the requests are launched and therefore the fixtures are not correct anymore.

I am not able to recreate the fixtures as that requires a proper amqp api. My suggestion is to recreate the fixtures and we can double check whether they make sense or not.

tete17 avatar May 02 '24 20:05 tete17

Would be great to make those all those sleeps in go-api configurable.

We have a plan to import go-api into this repository. Maybe we should to do that before continuing with this PR?

dentarg avatar May 02 '24 20:05 dentarg

My findings for today.

  1. terraform-json still get error with v0.12.0, just used latest version. Only did some quick searches and bumped it yesterday. The error I get with v0.12.0.
Got error running Terraform: unsupported state format version: expected ["0.1" "0.2"], got "1.0"
  1. It seems to be calling the ValidateChange("plan") function for an instance more times than before.

Yes, doing a re-recording both ValidateChanges are called three times now, before two. Even with two, it's one time more than needed. Unless it has to do with the customdiff used and both are triggered. The extra call that is missing, is the reason for the error about missing interactions.

  1. There is a lot of artificial waits

Yes, more and more of them we have made configurable. For the test where wait been an issue, there is a save hook removing these requests from the fixtures. https://github.com/cloudamqp/terraform-provider-cloudamqp/blob/main/cloudamqp/provider_test.go#L79-L154.

In case of the basic alarm test, that wait is not being waited for. Finished in 11.8 ms according to the fixture. We also use the flag SkipRequestLatency for the recorder. https://github.com/cloudamqp/terraform-provider-cloudamqp/blob/main/cloudamqp/provider_test.go#L59. Should not simulate the durations.

Compared the time for TestAccAlarm_Basic between the SDK versions, before about 0.2 s now 6.4 s.

  1. what deprecated function to select the provider does it still use I can't find it.

Saw you found it here: https://github.com/tete17/terraform-provider-cloudamqp/blob/Update-to-sdk-v2/cloudamqp/provider_test.go#L158-L161 :)

tbroden84 avatar May 03 '24 09:05 tbroden84

Hey @tbroden84 have you tried my branch?

I think it is using 0.12.0 and I am not seeing any of the "0.1", "0.2" errors. I wonder why.

I think the waits are triggered because of the wrong fixtures. The order in which terraform does requests has clearlly changed and that probablly leads to the go-api package to get results that trigger the artificial wait.

Can you regenerate the fixtures using my code please? I think that will fix all the issues we have.

tete17 avatar May 03 '24 17:05 tete17

The order in which terraform does requests has clearlly changed

It would be good to fully understand this (what changed in Terraform?)

dentarg avatar May 03 '24 19:05 dentarg