cloudflare_ip_list import set correct account_id

[seankhliao]

The account id is set on L100 but overriden with the empty string (because it's an import action) on L109

terraform-provider-cloudflare/cloudflare/resource_cloudflare_ip_list.go

Lines 100 to 109 in 4b7fbd9

	client.AccountID = accountID
	resourceCloudflareIPListRead(d, meta)
	return []*schema.ResourceData{d}, nil
	}
	func resourceCloudflareIPListRead(d *schema.ResourceData, meta interface{}) error {
	client := meta.(*cloudflare.API)
	client.AccountID = d.Get("account_id").(string)

Fixes #915

[jacobbednarz]

I don't think this is the solution to this issue as will not populate the client.AccountID value for the Create or Update methods. Instead, I think you need to first check if the client.AccountID is empty and then fetch the value if it is not.

func resourceCloudflareIPListRead(d *schema.ResourceData, meta interface{}) error {
	client := meta.(*cloudflare.API)

  if client.AccountID == "" {
    client.AccountID = d.Get("account_id").(string)
  }

// .. snip

[seankhliao]

how about this?

func resourceCloudflareIPListImport(d *schema.ResourceData, meta interface{}) ([]*schema.ResourceData, error) {
        // ... snip
 	accountID, listID := attributes[0], attributes[1]
	d.SetId(listID)
        d.Set("account_id", accountID) // replace client.AccountID = accountID 
       // ... snip
}
        

[jacobbednarz]

The change seems like a better outcome. Would you mind adding a test to verify the Import function works as expected?

[seankhliao]

I looked, but didn't really understand how the test setup was supposed to work even after looking at the other import tests

[jacobbednarz]

Sure, let me explain. For Import tests, we are generally looking to confirm that we are able to fetch the remote state and store it within the Terraform state file. The way to do this within the testing setup is generally though ImportStateVerify: true within the test step. Here is an example:

package cloudflare

import (
	"fmt"
	"os"
	"testing"

	"github.com/hashicorp/terraform-plugin-sdk/helper/resource"
)

func TestAccCloudflareIPList_Import(t *testing.T) {
	rnd := generateRandomResourceName()
	accountID := os.Getenv("CLOUDFLARE_ACCOUNT_ID")
	name := "cloudflare_ip_list." + rnd

	resource.Test(t, resource.TestCase{
		PreCheck:  func() { testAccPreCheck(t) },
		Providers: testAccProviders,
		Steps: []resource.TestStep{
			{
				Config: testAccCheckCloudflareIPList(rnd, rnd, rnd, accountID),
			},
			{
				ResourceName:        name,
				ImportStateIdPrefix: fmt.Sprintf("%s/", accountID),
				ImportState:         true,
				ImportStateVerify:   true,
			},
		},
	})
}

Running this on the master branch, you can see the same error you encountered as the API call failed and it was returned (unsuccessful terraform apply).

=== RUN   TestAccCloudflareIPList_Import
    testing.go:705: Step 1 error: error reading IP List with ID "0da4ecbc874e45a0939ad890de54c3fc": error from makeRequest: HTTP status 400: content "{\"success\":false,\"errors\":[{\"code\":7003,\"message\":\"Could not route to \\/accounts\\/rules\\/lists\\/0da4ecbc874e45a0939ad890de54c3fc, perhaps your object identifier is invalid?\"},{\"code\":7000,\"message\":\"No route for that URI\"}],\"messages\":[],\"result\":null}"
--- FAIL: TestAccCloudflareIPList_Import (3.77s)
FAIL
FAIL	github.com/cloudflare/terraform-provider-cloudflare/cloudflare	4.259s
FAIL

However, with your fix:

=== RUN   TestAccCloudflareIPList_Import
--- PASS: TestAccCloudflareIPList_Import (4.44s)
PASS
ok  	github.com/cloudflare/terraform-provider-cloudflare/cloudflare	5.005s

And because of the ImportStateVerify, we can be sure the resource was able to be stored. Note: other things may go wrong but that then comes down to individual attribute coverage as opposed to something generic you've found here.

Full details on the Import attributes for TestStep can be found at https://github.com/hashicorp/terraform-plugin-sdk/blob/9f0df37a8fdb2627ae32db6ceaf7f036d89b6768/helper/resource/testing.go#L345-L386

I'll push up the changes to include this in your branch and we'll get this landed.

[jacobbednarz]

this has been released in v2.18.0