Update indexes when index definitions change

[adrienthebo]

The null_resource creating and cleaning up indexes was triggering when the filename changed, but the filename in question was a temporary file whose name wouldn't change with the content. This commit changes the triggering to occur when the index content changes.

This pull request is based on #5. If needed it can be refactored to be an independent pull request, otherwise it should be merged after #5 is merged.

[morgante]

It looks like setup is still failing for me:

bash-4.3# kitchen converge
-----> Starting Kitchen (v1.23.2)
$$$$$$ Running command `terraform version` in directory /cftk/workdir
       Terraform v0.11.8

$$$$$$ Terraform v0.11.8 is supported
-----> Converging <default-local>...
$$$$$$ Running command `terraform workspace select kitchen-terraform-default-local` in directory /cftk/workdir/test/fixtures
$$$$$$ Running command `terraform get -update` in directory /cftk/workdir/test/fixtures
       - module.datastore
         Updating source "../../"
$$$$$$ Running command `terraform validate -check-variables=true   ` in directory /cftk/workdir/test/fixtures
$$$$$$ Running command `terraform apply -lock=true -lock-timeout=0s -input=false -auto-approve=true  -parallelism=10 -refresh=true  ` in directory /cftk/workdir/test/fixtures
       local_file.cloud-datastore-index-file: Refreshing state... (ID: 087712ad531b642a3f898a9e704113d8ea8d9e0b)
       null_resource.cloud-datastore-indices: Refreshing state... (ID: 6597675684863530123)
       null_resource.gcloud-activate-service-account: Refreshing state... (ID: 2079918858837321711)
       module.datastore.null_resource.cloud-datastore-indices: Destroying... (ID: 6597675684863530123)
       module.datastore.null_resource.cloud-datastore-indices: Destruction complete after 0s
       module.datastore.null_resource.cloud-datastore-indices: Creating...
         triggers.%:                     "" => "1"
         triggers.changes_in_index_file: "" => "087712ad531b642a3f898a9e704113d8ea8d9e0b"
       module.datastore.null_resource.cloud-datastore-indices: Provisioning with 'local-exec'...
       module.datastore.null_resource.cloud-datastore-indices (local-exec): Executing: ["/bin/sh" "-c" "gcloud datastore indexes cleanup /cftk/workdir/test/fixtures/.terraform/modules/022b107b5d0ba7739fe6905f7952d260/tmp/index.yaml --project=clf-tf-dev"]
       module.datastore.null_resource.cloud-datastore-indices (local-exec): Configurations to update:

       module.datastore.null_resource.cloud-datastore-indices (local-exec): descriptor:      [/cftk/workdir/test/fixtures/.terraform/modules/022b107b5d0ba7739fe6905f7952d260/tmp/index.yaml]
       module.datastore.null_resource.cloud-datastore-indices (local-exec): type:            [datastore indexes]
       module.datastore.null_resource.cloud-datastore-indices (local-exec): target project:  [clf-tf-dev]

       module.datastore.null_resource.cloud-datastore-indices (local-exec): Do you want to continue (Y/n)?
       module.datastore.null_resource.cloud-datastore-indices (local-exec): ERROR: (gcloud.datastore.indexes.cleanup) You do not currently have an active account selected.
       module.datastore.null_resource.cloud-datastore-indices (local-exec): Please run:

       module.datastore.null_resource.cloud-datastore-indices (local-exec):   $ gcloud auth login

       module.datastore.null_resource.cloud-datastore-indices (local-exec): to obtain new credentials, or if you have already logged in with a
       module.datastore.null_resource.cloud-datastore-indices (local-exec): different account:

       module.datastore.null_resource.cloud-datastore-indices (local-exec):   $ gcloud config set account ACCOUNT

       module.datastore.null_resource.cloud-datastore-indices (local-exec): to select an already authenticated account to use.

       Error: Error applying plan:

       1 error(s) occurred:

       * module.datastore.null_resource.cloud-datastore-indices: Error running command 'gcloud datastore indexes cleanup /cftk/workdir/test/fixtures/.terraform/modules/022b107b5d0ba7739fe6905f7952d260/tmp/index.yaml --project=clf-tf-dev': exit status 1. Output: Configurations to update:

       descriptor:      [/cftk/workdir/test/fixtures/.terraform/modules/022b107b5d0ba7739fe6905f7952d260/tmp/index.yaml]
       type:            [datastore indexes]
       target project:  [clf-tf-dev]

       Do you want to continue (Y/n)?  
       ERROR: (gcloud.datastore.indexes.cleanup) You do not currently have an active account selected.
       Please run:

         $ gcloud auth login

       to obtain new credentials, or if you have already logged in with a
       different account:

         $ gcloud config set account ACCOUNT

       to select an already authenticated account to use.

       Terraform does not automatically rollback in the face of errors.
       Instead, your Terraform state file has been partially updated with
       any resources that successfully completed. Please address the error
       above and apply again to incrementally change your infrastructure.

>>>>>> ------Exception-------
>>>>>> Class: Kitchen::ActionFailed
>>>>>> Message: 1 actions failed.
>>>>>>     Converge failed on instance <default-local>.  Please see .kitchen/logs/default-local.log for more details
>>>>>> ----------------------
>>>>>> Please see .kitchen/logs/kitchen.log for more details
>>>>>> Also try running `kitchen diagnose --all` for configuration

Two updates:

1. Instead of having the service account activation happen in a null_resource it should happen via an environment variable like how we do it in project factory: https://github.com/terraform-google-modules/terraform-google-project-factory/blob/master/scripts/delete-service-account.sh#L23
2. We should probably move all the gcloud executions into their own script(s), like how we do for the other modules.

[adrienthebo]

1. Working on it now.
2. Since the provisioner logic amounts to one liner scripts, I think the value gained by extracting scripts is minimal. What other benefits do we get from having scripts, aside from avoiding shell scripts embedded in Terraform?

[adrienthebo]

I've changed the Terraform configuration to override the CLOUDSDK_AUTH_CREDENTIAL_FILE_OVERRIDE variable, since it had the smaller footprint - if you'd rather this be done with independent shell scripts I can switch it out accordingly.

[morgante]

Since the provisioner logic amounts to one liner scripts, I think the value gained by extracting scripts is minimal. What other benefits do we get from having scripts, aside from avoiding shell scripts embedded in Terraform?

• The shell scripts go through our linter, which will detect some bash incompatibilities/potential bugs (this is the biggest reason)
• We can test them independently
• I think they're easier to develop/iterate on as separate files

[adrienthebo]

@morgante those reasons make a lot of sense, thanks for explaining that! I've extracted scripts per your original request.

In addition it looks like there might be a race condition with the provisioner called on create; I'm having issues where the script fails because it can't create indexes before fully deleting existing indexes. I suspect that adding some retry + backoff logic may solve this problem; let me know if it affects you as well and I can investigate this further.

[morgante]

Thanks for updating. Sadly it looks like tests are still failing for me:

bash-4.3# kitchen converge
-----> Starting Kitchen (v1.23.2)
$$$$$$ Running command `terraform version` in directory /cftk/workdir
       Terraform v0.11.8

$$$$$$ Terraform v0.11.8 is supported
-----> Converging <default-local>...
$$$$$$ Running command `terraform workspace select kitchen-terraform-default-local` in directory /cftk/workdir/test/fixtures
$$$$$$ Running command `terraform get -update` in directory /cftk/workdir/test/fixtures
       - module.datastore
         Updating source "../../"
$$$$$$ Running command `terraform validate -check-variables=true   ` in directory /cftk/workdir/test/fixtures
$$$$$$ Running command `terraform apply -lock=true -lock-timeout=0s -input=false -auto-approve=true  -parallelism=10 -refresh=true  ` in directory /cftk/workdir/test/fixtures
       local_file.cloud-datastore-index-file: Refreshing state... (ID: 087712ad531b642a3f898a9e704113d8ea8d9e0b)
       null_resource.gcloud-activate-service-account: Refreshing state... (ID: 2079918858837321711)
       null_resource.cloud-datastore-indices: Refreshing state... (ID: 2956249769961964373)
       module.datastore.null_resource.cloud-datastore-indices: Destroying... (ID: 2956249769961964373)
       module.datastore.null_resource.gcloud-activate-service-account: Destroying... (ID: 2079918858837321711)
       module.datastore.null_resource.cloud-datastore-indices: Destruction complete after 0s
       module.datastore.null_resource.gcloud-activate-service-account: Destruction complete after 0s
       module.datastore.null_resource.cloud-datastore-indices: Creating...
         triggers.%:                     "" => "1"
         triggers.changes_in_index_file: "" => "087712ad531b642a3f898a9e704113d8ea8d9e0b"
       module.datastore.null_resource.cloud-datastore-indices: Provisioning with 'local-exec'...
       module.datastore.null_resource.cloud-datastore-indices (local-exec): Executing: ["/bin/sh" "-c" "/cftk/workdir/test/fixtures/.terraform/modules/022b107b5d0ba7739fe6905f7952d260/scripts/create-indexes.sh '/cftk/workdir/sa-key.json' 'clf-tf-dev' '/cftk/workdir/test/fixtures/.terraform/modules/022b107b5d0ba7739fe6905f7952d260/tmp/index.yaml'"]
       module.datastore.null_resource.cloud-datastore-indices (local-exec): Configurations to update:

       module.datastore.null_resource.cloud-datastore-indices (local-exec): descriptor:      [/cftk/workdir/test/fixtures/.terraform/modules/022b107b5d0ba7739fe6905f7952d260/tmp/index.yaml]
       module.datastore.null_resource.cloud-datastore-indices (local-exec): type:            [datastore indexes]
       module.datastore.null_resource.cloud-datastore-indices (local-exec): target project:  [clf-tf-dev]

       module.datastore.null_resource.cloud-datastore-indices (local-exec): Configurations to update:

       module.datastore.null_resource.cloud-datastore-indices (local-exec): descriptor:      [/cftk/workdir/test/fixtures/.terraform/modules/022b107b5d0ba7739fe6905f7952d260/tmp/index.yaml]
       module.datastore.null_resource.cloud-datastore-indices (local-exec): type:            [datastore indexes]
       module.datastore.null_resource.cloud-datastore-indices (local-exec): target project:  [clf-tf-dev]

       module.datastore.null_resource.cloud-datastore-indices: Creation complete after 3s (ID: 2780862148350938652)

       Apply complete! Resources: 1 added, 0 changed, 2 destroyed.

       Outputs:

       project_id = clf-tf-dev
$$$$$$ Running command `terraform output -json` in directory /cftk/workdir/test/fixtures
       {
           "project_id": {
        "sensitive": false,
        "type": "string",
        "value": "clf-tf-dev"
           }
       }
       Finished converging <default-local> (0m4.61s).
-----> Kitchen is finished. (0m6.36s)
bash-4.3# kitchen verify
-----> Starting Kitchen (v1.23.2)
$$$$$$ Running command `terraform version` in directory /cftk/workdir
       Terraform v0.11.8

$$$$$$ Terraform v0.11.8 is supported
-----> Setting up <default-local>...
       Finished setting up <default-local> (0m0.00s).
-----> Verifying <default-local>...
       Loaded cloud-datastore 
[2018-09-27T19:28:02+00:00] WARN: Cannot find a UUID for your node.

Profile: Google Cloud Datastore (cloud-datastore)
Version: 0.1.0
Target:  local://

  Terraform outputs (/cftk/workdir/test/fixtures)
     ✔  should exist
     ✔  project_id should eq "clf-tf-dev"
  Command: `gcloud --project='clf-tf-dev' datastore indexes list --filter='properties[0].name=done' --format=json --quiet | jq -jce '.[0].kind'`
     ✔  exit_status should eq 0
     ×  stdout should eq "Task"

     expected: "Task"
          got: ""

     (compared using ==)

  Command: `gcloud --project='clf-tf-dev' datastore indexes list --filter='properties[0].name=done' --format=json --quiet | jq -jce '.[0].properties'`
     ✔  exit_status should eq 0
     ×  stdout should eq "[{\"direction\":\"ASCENDING\",\"name\":\"done\"},{\"direction\":\"DESCENDING\",\"name\":\"priority\"}]"

     expected: "[{\"direction\":\"ASCENDING\",\"name\":\"done\"},{\"direction\":\"DESCENDING\",\"name\":\"priority\"}]"
          got: ""

     (compared using ==)

  Command: `gcloud --project='clf-tf-dev' datastore indexes list --filter='properties[0].name=collaborators' --format=json --quiet | jq -jce '.[0].kind'`
     ✔  exit_status should eq 0
     ×  stdout should eq "Task"

     expected: "Task"
          got: ""

     (compared using ==)

  Command: `gcloud --project='clf-tf-dev' datastore indexes list --filter='properties[0].name=collaborators' --format=json --quiet | jq -jce '.[0].properties'`
     ✔  exit_status should eq 0
     ×  stdout should eq "[{\"direction\":\"ASCENDING\",\"name\":\"collaborators\"},{\"direction\":\"DESCENDING\",\"name\":\"created\"}]"

     expected: "[{\"direction\":\"ASCENDING\",\"name\":\"collaborators\"},{\"direction\":\"DESCENDING\",\"name\":\"created\"}]"
          got: ""

     (compared using ==)

[adrienthebo]

@morgante I just reproduced this by running kitchen verify in the docker container without CLOUDSDK_AUTH_CREDENTIAL_FILE_OVERRIDE set. If we add an assertion that stderr is empty we get a more informative error:

  Command: `gcloud --project='cft-datastore-fixture-dev-27eb' datastore indexes list --filter='properties[0].name=collaborators' --format=json --quiet | jq -jce '.[0].properties'`
     ✔  exit_status should eq 0
     ×  stderr should eq ""
     expected: ""
          got: "ERROR: (gcloud.datastore.indexes.list) You do not currently have an active account selected.\nPlease...t:\n\n  $ gcloud config set account ACCOUNT\n\nto select an already authenticated account to use.\n"
     (compared using ==)
     Diff:
     @@ -1 +1,12 @@
     +ERROR: (gcloud.datastore.indexes.list) You do not currently have an active account selected.
     +Please run:
     +
     +  $ gcloud auth login
     +
     +to obtain new credentials, or if you have already logged in with a
     +different account:
     +
     +  $ gcloud config set account ACCOUNT
     +
     +to select an already authenticated account to use.

I'll add a new commit adding the stderr check. In the mean time could you re-run the tests with something like the following environment variables set?

export TF_VAR_credentials_file_path="./credentials.json"
export TF_VAR_project="cft-datastore-fixture-dev-27eb"

export CLOUDSDK_AUTH_CREDENTIAL_FILE_OVERRIDE="./credentials.json"

[morgante]

@adrienthebo I'm confused about the need to set CLOUDSDK_AUTH_CREDENTIAL_FILE_OVERRIDE in this case? Why do I need to manually override that (assuming TF_VAR_credentials_file_path is set correctly)?

The goal is I should only have to configure the module itself and not worry about the underlying gcloud shelling. The fact that tests break without that additional env variable points to a broken module.

[adrienthebo]

In https://github.com/terraform-google-modules/terraform-google-cloud-datastore/pull/6/commits/ea937c39f78141e500a548147413cdf1a899b2b8 I converted the tests to reuse the credentials given to Terraform. The reasoning for this is that if the credentials can be used to create the datastore indices then they should be usable for verifying that the indices are present in the tests, and if the credentials can't be used for tests then kitchen converge will fail.

Per our earlier conversation there are some scenarios where this approach could break, but I wasn't able to follow the reasoning. Do we want to explore the cases where invalid credentials were given to the module? Is there a case where the CLOUDSDK_AUTH_CREDENTIAL_OVERRIDE leaks into the module, or where that variable isn't set so we inadvertently fall back to other gcloud credentials?

[morgante]

@adrienthebo This looks good.