Fix import of bigquery transfer config location

[obada-ab]

Fix the custom import code for bigquery transfer config, and make it import the location properly.

Fixes hashicorp/terraform-provider-google#12011

If this PR is for Terraform, I acknowledge that I have:

• Searched through the issue tracker for an open issue that this either resolves or contributes to, commented on it to claim it, and written "fixes {url}" or "part of {url}" in this PR description. If there were no relevant open issues, I opened one and commented that I would like to work on it (not necessary for very small changes).
• Ensured that all new fields I added that can be set by a user appear in at least one example (for generated resources) or third_party test (for handwritten resources or update tests).
• Generated Terraform providers, and ran make test and make lint in the generated providers to ensure it passes unit and linter tests.
• Ran relevant acceptance tests using my own Google Cloud project and credentials (If the acceptance tests do not yet pass or you are unable to run them, please let your reviewer know).
• Read Write release notes before writing my release note below.

Release Note Template for Downstream PRs (will be copied)

bigquerydatatransfer: fixed a bug when importing 'location' of 'google_bigquery_data_transfer_config'

[modular-magician]

Hello! I am a robot. It looks like you are a: Community Contributor Googler Core Contributor. Tests will run automatically.

@roaks3, a repository maintainer, has been assigned to review your changes. If you have not received review feedback within 2 business days, please leave a comment on this PR asking them to take a look.

You can help make sure that review is quick by doing a self-review and by running impacted tests locally.

[modular-magician]

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 1 file changed, 13 insertions(+))
Terraform Beta: Diff ( 1 file changed, 13 insertions(+))

[obada-ab]

Tested the change manually by following: https://googlecloudplatform.github.io/magic-modules/develop/run-tests/#optional-test-manually
then ran terraform import for a transfer config in my gcp project, and checked location inside terraform.tfstate

[olucasfagundes]

Hello @obada-ab, please, have you noticed if data_refresh_window_days and the disabled status are imported? I noticed that terraform import it is generating this fields as null too and the terraform plan shows that terraform will try to recreate the resources (destroy and apply).

[modular-magician]

Tests analytics

Total tests: 2988
Passed tests 2691
Skipped tests: 296
Affected tests: 1

Action taken

Found 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests

TestAccSpannerDatabaseIamPolicy

Get to know how VCR tests work

[modular-magician]

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccSpannerDatabaseIamPolicy[Debug log]

Rerun these tests in REPLAYING mode to catch issues

$\textcolor{green}{\textsf{No issues found for passed tests after REPLAYING rerun.}}$

---

$\textcolor{green}{\textsf{All tests passed!}}$
View the build log or the debug log for each test

[obada-ab]

Hello @obada-ab, please, have you noticed if data_refresh_window_days and the disabled status are imported? I noticed that terraform import it is generating this fields as null too and the terraform plan shows that terraform will try to recreate the resources (destroy and apply).

Hey @olucasfagundes. I did some testing and both disabled and data_refresh_window_days are being imported correctly for me. If this problem is persistent for you please open a new issue and mention the steps you've taken and the version you are using.

[obada-ab]

@roaks3 can you take a look, thanks

[roaks3]

@obada-ab the issue mentions using import with just the config_id, so I just want to make sure we aren't breaking any functionality that used to be working. Were you able to verify that this was not possible to use before your fix (ie. all 6 parts were required for import)?

[obada-ab]

@obada-ab the issue mentions using import with just the config_id, so I just want to make sure we aren't breaking any functionality that used to be working. Were you able to verify that this was not possible to use before your fix (ie. all 6 parts were required for import)?

@roaks3 Thanks for taking a look, I tested import with just the config ID but it didn't work. However, importing in the form projects/{project}/transferConfigs/{config_id} does work before the code changes here.

I'll work on addressing this.

[modular-magician]

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 2 files changed, 18 insertions(+), 4 deletions(-))
Terraform Beta: Diff ( 2 files changed, 18 insertions(+), 4 deletions(-))

[modular-magician]

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 2 files changed, 18 insertions(+), 4 deletions(-))
Terraform Beta: Diff ( 2 files changed, 18 insertions(+), 4 deletions(-))

[modular-magician]

Tests analytics

Total tests: 3007
Passed tests 2711
Skipped tests: 296
Affected tests: 0

$\textcolor{green}{\textsf{All tests passed in REPLAYING mode.}}$
View the build log

[obada-ab]

@roaks3 made changes so that location is only imported if it's included in name, manually tested importing both name formats are working correctly (api).

I can't think of any simple way to import the location property if it is not included in name, what do you think?

[roaks3]

Hmm ok, after a bit more looking, I'm not completely clear on the behavior when a location is not present. It seems as though this Terraform resource treats no location as meaning US, and I don't know how the API treats no location. Possibly it could mean US, any/unspecified location, or global. Additionally, is there any way to retrieve the location (I don't see one currently)?

I think that worst case, we fix the issue when the location is present in the import id, and when it isn't we just make sure we have the same behavior as before. In some sense, the projects/{projectId}/transferConfigs/{configId} format may not actually be a valid import id for this resource.

[obada-ab]

@roaks3 I think the API considers having no location in name to be equal to US. I tried the HTTP GET method using the short form projects/{projectId}/transferConfigs/{configId} for a transfer config that is in asia-northeast1, and I got this response:

curl -X GET -H "Authorization: Bearer $(gcloud auth print-access-token)" -H "Content-Type: application/json" https://bigquerydatatransfer.googleapis.com/v1/projects/XX/transferConfigs/YY
{
  "error": {
    "code": 404,
    "message": "The requested transfer config (projects/XX/locations/us/transferConfigs/YY) was not found.",
    "status": "NOT_FOUND",
    "details": [
      {
        "@type": "type.googleapis.com/google.rpc.DebugInfo",
        "detail": "[ORIGINAL ERROR] generic::not_found: The requested transfer config (projects/XX/locations/us/transferConfigs/YY) was not found. [google.rpc.error_details_ext] { message: \"The requested transfer config (projects/XX/locations/us/transferConfigs/YY) was not found.\" }"
      }
    ]
  }
}

(notice how locations/us is added)

It seems as though this Terraform resource treats no location as meaning US...

This is true; however, the terraform default of US is only a client-side value and is not applied when performing an import. So if you import a transfer config in the US using the short form projects/{projectId}/transferConfigs/{configId} , the imported location is null, and when doing terraform plan you'd get this:

  # google_bigquery_data_transfer_config.query_config must be replaced
-/+ resource "google_bigquery_data_transfer_config" "query_config" {
      ~ id                       = "projects/XX/transferConfigs/YY" -> (known after apply)
      + location                 = "US" # forces replacement
      ~ name                     = "projects/XX/locations/us/transferConfigs/YY" -> (known after apply)
        # (8 unchanged attributes hidden)

        # (2 unchanged blocks hidden)
    }

So the issue still remains.

After looking into it, it seems that even when using the short projects/{projectId}/transferConfigs/{configId} import form, the name property inside the imported instance consistently contains locations/XX:

  "resources": [
    {
      "mode": "managed",
      "type": "google_bigquery_data_transfer_config",
      "name": "query_config",
      "provider": "provider[\"registry.terraform.io/hashicorp/google\"]",
      "instances": [
        {
          "schema_version": 0,
          "attributes": {
            .
            .
            .
            "id": "projects/XX/transferConfigs/YY",
            "location": null,
            "name": "projects/XX/locations/us/transferConfigs/YY",

If there's a way to extract the location from the imported name (and not the name given by the user in the terraform import command), I think this would be a decent solution. Otherwise we could also only explicitly import the location if it's provided, and default to the old behaviour otherwise. what do you think?

[obada-ab]

@roaks3 any updates?

I think the current changes make sense as they fix the customer issues, and keep the old behaviour for the case where the location is not present while importing.

I'm thinking about creating and tracking a separate issue for consistently importing the location. what do you think?

[roaks3]

Yea, I agree. This seems to be a safest set of changes.

When location is not present in the import ID, I think it might technically be more correct to set it to a hardcoded value of US, but we would need to be careful about what impacts that change would have. Creating a new issue would be great to capture some of those details.

[obada-ab]

@roaks3 I've created a separate issue: hashicorp/terraform-provider-google#15733