Repository: iterative/cml
Pull request #885 (2022-01-22 06:36:49)

specify TPI version
- Specify minimum terraform-provider-iterative version (may fix edge cases)
- follow-up to #883


0x2b3bfa0: 🤔 What edge cases is this change trying to cover?
0x2b3bfa0: /tests
casperdcl: Things related to https://github.com/iterative/terraform-provider-iterative/pull/266?
DavidGOrtega: We do not tie the latest version of terraform. Indeed we propagate a non versioned terraform state so the terraform installed in the cloud instance is able to run the state.

Just to clarify. We are not tied a a specific terraform version
dacbd: could be nice, in combo with: https://github.com/iterative/terraform-provider-iterative/pull/255 for users to have the option to lock their automation into a tested/proven release (for them).


For example, I had a failure, and at least one other user from discord where a new release of tpi was out that cml tried to use but the registry hadn't quite propagated completely and jobs failed, granted this was an edge case of some stars aligning just right with when a job was running / when a release was made.


Yes if not set, default to latest 💯 
dacbd: > But this PR the thing it does it to enforce that the tpi version is above a minimum and not to specify a specific terraform version, right? @dacbd
> 
> ```
> iterative = { source = "iterative/iterative", version = ">= 0.9.10" }
> ```

I confess to only reading the comments 🐍 
dacbd: my brain auto translated "specify TPI version" to "Allow user to specify TPI version"
casperdcl: I think at least letting the user specify the TPI version as @dacbd suggests is a good idea.

- TPI may introduce a breaking change in future
- there may be some time for a change to propagate to a CML patch release (e.g. #883)
- right now, if a user has already manually run `terraform apply` there may be an old cached version of TPI which CML will use, right?
DavidGOrtega: > I think at least letting the user specify the TPI version as @dacbd suggests is a good idea.

Just for the record CML unversion the tfstate from terraform. This is done to avoid minor terraform changes that avoids the run. 
Historically we have been always backwards compatible. Indeed `task` could be the first time that we are changing behaviour from version to version (however we are in alpha mode so probably in the future we will stick to the plan)
dacbd: > Just for the record CML unversion the tfstate from terraform.

@DavidGOrtega are you referring to when cml is invoked with base64 tfstate on the server?


DavidGOrtega: > @DavidGOrtega are you referring to when cml is invoked with base64 tfstate on the server?

Yes, one of the biggest issues was having synced the terraform version in the remote host and the user's local one. If the differ terraform will fail. This topic was a hell back in the day for terraform users that discovered unpleasantly that they were not able to terminate instances because they updated terraform
DavidGOrtega: In any case. Does this PR solve the problem? Should we merge it? Should we close it? @iterative/cml 
dacbd: I'm happy to add the selection feature I want to see as a new PR or as one targeting this branch (convert this PR to draft for now?)
dacbd: @iterative/cml this half works. the invoking cml can have its tpi specified via passing a [version constraint]( https://www.terraform.io/language/expressions/version-constraints) with `--tpi-version >= 0.9.10` / `--tpi-version =0.10.3` but the instances installed version of the provider is "latest" I haven't investigated changing that yet, but my hunch is it would > non-trivial

I feel this doesn't quite match the "expected behavior" but on the other hand the instance's tpi only preforms the delete action and none of the setup/configuration... 👍 👎 

0x2b3bfa0: > _I haven't investigated changing that yet, but my hunch is it would > non-trivial:

From what I understand, something along the lines of https://github.com/iterative/terraform-provider-iterative/pull/103 should suffice to close the [stange loop](https://en.wikipedia.org/wiki/Strange_loop).
dacbd: Yeah, that was my initial thought about how to get it there as well but feels extra redundant, you pick your version or constraint which is part of the provider definition but also pass in the resource definition?

I would want to try embedding it [here](https://github.com/iterative/terraform-provider-iterative/blob/master/iterative/resource_runner.go#L399) so that it is communicated to the instance via base64 tfstate. /my "haven't investigated" part
dacbd: > Yeah, that was my initial thought about how to get it there as well but feels extra redundant, you pick your version or constraint which is part of the provider definition but also pass in the resource definition?
> 
> I would want to try embedding it [here](https://github.com/iterative/terraform-provider-iterative/blob/master/iterative/resource_runner.go#L399) so that it is communicated to the instance via base64 tfstate. /my "haven't investigated" part

being able to parse it out of the tfstate like that would make it a simple adjustment here:
https://github.com/iterative/cml/blob/01b2cef37b3efaf2241d0ee945ccc71662a9c3d2/bin/cml/runner.js#L375-L383
as a param to line 375
0x2b3bfa0: Not sure if it's going to work, but definitely worth a try. Good initiation in the path of the Terraform Heresy. 🎉 😄 
dacbd: ahh yes, tested via:
```
bin/cml.js runner \
    --single \
    --tpi-version =0.10.3 \
    --labels=tpi_version \
    --token=*** \
    --cloud=aws \
    --cloud-region=us-west-2 \
    --cloud-type=t3.small
```

Cat of the generated main.tf / lock file

```
provider "registry.terraform.io/iterative/iterative" {
  version     = "0.10.3"
  constraints = "0.10.3"
  hashes = [
    "h1:qG6mYJ7/v/RvSvAefma7xdvrzxViyIvP6K35IjvlPU0=",
    "zh:33e6f437aeae3834f5ac34c5b39982dcd118169c039232d327e5dae17faa4283",
    "zh:8f72ca6dad3fdffbb7a5f267ac8d1d0d452a4e31e8764a54aa17d55fcf126637",
    "zh:975b4511e2592e46e532b6d2e9a7d57763592742f968ad3100021ccff531102b",
    "zh:a634f4d77634b5cc5f0599a3688b8bc363d9f2889086671d6f07a57421c7d15d",
    "zh:b4b648188a85c77c7285247d9830cf6ce0ac4c67[41](https://github.com/dacbd/cml-pulse/runs/6010390374?check_suite_focus=true#step:6:41)5dc4e6b0794f3d7c950ad7",
    "zh:cc5624f08b9b1e[45](https://github.com/dacbd/cml-pulse/runs/6010390374?check_suite_focus=true#step:6:45)14fac5b324c150713c2b9e63c7f3874f37792e125ca72aed",
    "zh:e0707971f3cf1c0e011290e6e76a05b03183f756c91b5b9bd9c5c5027c8b2353",
    "zh:e5bb7ad0fae6d50ca04109cfaa57ef[47](https://github.com/dacbd/cml-pulse/runs/6010390374?check_suite_focus=true#step:6:47)a263fd6eb00eec[51](https://github.com/dacbd/cml-pulse/runs/6010390374?check_suite_focus=true#step:6:51)[61](https://github.com/dacbd/cml-pulse/runs/6010390374?check_suite_focus=true#step:6:61)0[62](https://github.com/dacbd/cml-pulse/runs/6010390374?check_suite_focus=true#step:6:62)3fecbb06c5e",
  ]
}
terraform {
  required_providers {
    iterative = { source = "iterative/iterative", version = "=0.10.3" }
  }
}
provider "iterative" {}

resource "iterative_cml_runner" "runner" {
  repo = "https://github.com/dacbd/cml-pulse"
  token = "***"
  driver = "github"
  labels = "tpi_version"
  cml_version = "0.14.1"
  idle_timeout = 300
  name = "cml-gjhlm6lw9j"
  single = "true"
  cloud = "aws"
  region = "us-west-2"
  instance_type = "t3.small"
  
  
  
  
  
  spot_price = -1
  
  
  
  metadata = {
    
  }
  docker_volumes = []
}
```

Patch:
diff --git a/bin/cml/runner.js b/bin/cml/runner.js
index 64875cc..db2420a 100755
--- a/bin/cml/runner.js
+++ b/bin/cml/runner.js
@@ -121,6 +121,7 @@ const runCloud = async (opts) => {
 
     const { token, repo, driver } = cml;
     const {
+      tpiVersion,
       labels,
       idleTimeout,
       name,
@@ -156,6 +157,7 @@ const runCloud = async (opts) => {
           'GPU model "tesla" has been deprecated; please use "v100" instead.'
         );
       tpl = tf.iterativeCmlRunnerTpl({
+        tpiVersion,
         repo,
         token,
         driver,
@@ -345,6 +347,7 @@ const run = async (opts) => {
 
   opts.workdir = opts.workdir || `${homedir()}/.cml/${opts.name}`;
   const {
+    tpiVersion,
     driver,
     repo,
     token,
@@ -372,7 +375,7 @@ const run = async (opts) => {
 
     await fs.mkdir(tfPath, { recursive: true });
     const tfMainPath = join(tfPath, 'main.tf');
-    const tpl = tf.iterativeProviderTpl();
+    const tpl = tf.iterativeProviderTpl({ tpiVersion });
     await fs.writeFile(tfMainPath, tpl);
     await tf.init({ dir: tfPath });
     await tf.apply({ dir: tfPath });
@@ -436,6 +439,12 @@ exports.handler = async (opts) => {
 exports.builder = (yargs) =>
   yargs.env('CML_RUNNER').options(
     kebabcaseKeys({
+      tpiVersion: {
+        type: 'string',
+        default: '>= 0.9.10',
+        description:
+          'Pin the iterative/iterative terraform provider to a specific version. i.e. "= 0.10.4" See: https://www.terraform.io/language/expressions/version-constraints'
+      },
       dockerVolumes: {
         type: 'array',
         default: [],
diff --git a/src/terraform.js b/src/terraform.js
index dbcb2a4..a5aa169 100644
--- a/src/terraform.js
+++ b/src/terraform.js
@@ -51,16 +51,15 @@ const destroy = async (opts = {}) => {
 const mapCloudMetadata = (metadata) =>
   Object.entries(metadata).map(([key, value]) => `${key} = "${value || ''}"`);
 
-const iterativeProviderTpl = () => {
-  return `
-terraform {
+const iterativeProviderTpl = (opts = {}) => {
+  const { tpiVersion } = opts;
+  return `terraform {
   required_providers {
-    iterative = {
-      source = "iterative/iterative"
-    }
+    iterative = { source = "iterative/iterative"${
+      tpiVersion ? `, version = "${tpiVersion}"` : ''
+    } }
   }
 }
-
 provider "iterative" {}
 `;
 };
@@ -91,9 +90,7 @@ const iterativeCmlRunnerTpl = (opts = {}) => {
     dockerVolumes
   } = opts;
 
-  const template = `
-${iterativeProviderTpl()}
-
+  const template = `${iterativeProviderTpl(opts)}
 resource "iterative_cml_runner" "runner" {
   ${repo ? `repo = "${repo}"` : ''}
   ${token ? `token = "${token}"` : ''}


Test patch:
diff --git a/bin/cml/runner.test.js b/bin/cml/runner.test.js
index f0d05e5..739faaa 100644
--- a/bin/cml/runner.test.js
+++ b/bin/cml/runner.test.js
@@ -63,6 +63,12 @@ describe('CML e2e', () => {
         --version                                 Show version number        [boolean]
         --log                                     Maximum log level
                 [string] [choices: \\"error\\", \\"warn\\", \\"info\\", \\"debug\\"] [default: \\"info\\"]
+        --tpi-version                             Pin the iterative/iterative
+                                                  terraform provider to a specific
+                                                  version. i.e. \\"= 0.10.4\\" See:
+                                                  https://www.terraform.io/language/ex
+                                                  pressions/version-constraints
+                                                       [string] [default: \\">= 0.9.10\\"]
         --docker-volumes                          Docker volumes. This feature is only
                                                   supported in GitLab
                                                                  [array] [default: []]
diff --git a/src/terraform.test.js b/src/terraform.test.js
index c906139..654600a 100644
--- a/src/terraform.test.js
+++ b/src/terraform.test.js
@@ -4,19 +4,13 @@ describe('Terraform tests', () => {
   test('default options', async () => {
     const output = iterativeCmlRunnerTpl({});
     expect(output).toMatchInlineSnapshot(`
-      "
-
-      terraform {
+      "terraform {
         required_providers {
-          iterative = {
-            source = \\"iterative/iterative\\"
-          }
+          iterative = { source = \\"iterative/iterative\\" }
         }
       }
-
       provider \\"iterative\\" {}
 
-
       resource \\"iterative_cml_runner\\" \\"runner\\" {
         
         
@@ -65,19 +59,13 @@ describe('Terraform tests', () => {
       awsSecurityGroup: 'mysg'
     });
     expect(output).toMatchInlineSnapshot(`
-      "
-
-      terraform {
+      "terraform {
         required_providers {
-          iterative = {
-            source = \\"iterative/iterative\\"
-          }
+          iterative = { source = \\"iterative/iterative\\" }
         }
       }
-
       provider \\"iterative\\" {}
 
-
       resource \\"iterative_cml_runner\\" \\"runner\\" {
         repo = \\"https://\\"
         token = \\"abc\\"
@@ -125,19 +113,13 @@ describe('Terraform tests', () => {
       spotPrice: '0.0001'
     });
     expect(output).toMatchInlineSnapshot(`
-      "
-
-      terraform {
+      "terraform {
         required_providers {
-          iterative = {
-            source = \\"iterative/iterative\\"
-          }
+          iterative = { source = \\"iterative/iterative\\" }
         }
       }
-
       provider \\"iterative\\" {}
 
-
       resource \\"iterative_cml_runner\\" \\"runner\\" {
         repo = \\"https://\\"
         token = \\"abc\\"
@@ -186,19 +168,13 @@ describe('Terraform tests', () => {
       metadata: { one: 'value', two: null }
     });
     expect(output).toMatchInlineSnapshot(`
-      "
-
-      terraform {
+      "terraform {
         required_providers {
-          iterative = {
-            source = \\"iterative/iterative\\"
-          }
+          iterative = { source = \\"iterative/iterative\\" }
         }
       }
-
       provider \\"iterative\\" {}
 
-
       resource \\"iterative_cml_runner\\" \\"runner\\" {
         repo = \\"https://\\"
         token = \\"abc\\"
@@ -250,19 +226,13 @@ describe('Terraform tests', () => {
       dockerVolumes: ['/aa:/aa', '/bb:/bb']
     });
     expect(output).toMatchInlineSnapshot(`
-      "
-
-      terraform {
+      "terraform {
         required_providers {
-          iterative = {
-            source = \\"iterative/iterative\\"
-          }
+          iterative = { source = \\"iterative/iterative\\" }
         }
       }
-
       provider \\"iterative\\" {}
 
-
       resource \\"iterative_cml_runner\\" \\"runner\\" {
         repo = \\"https://\\"
         token = \\"abc\\"
@@ -312,19 +282,13 @@ describe('Terraform tests', () => {
       awsSecurityGroup: 'mysg'
     });
     expect(output).toMatchInlineSnapshot(`
-      "
-
-      terraform {
+      "terraform {
         required_providers {
-          iterative = {
-            source = \\"iterative/iterative\\"
-          }
+          iterative = { source = \\"iterative/iterative\\" }
         }
       }
-
       provider \\"iterative\\" {}
 
-
       resource \\"iterative_cml_runner\\" \\"runner\\" {
         repo = \\"https://\\"
         token = \\"abc\\"
@@ -373,19 +337,13 @@ describe('Terraform tests', () => {
       startupScript: 'c3VkbyBlY2hvICdoZWxsbyB3b3JsZCcgPj4gL3Vzci9oZWxsby50eHQ='
     });
     expect(output).toMatchInlineSnapshot(`
-      "
-
-      terraform {
+      "terraform {
         required_providers {
-          iterative = {
-            source = \\"iterative/iterative\\"
-          }
+          iterative = { source = \\"iterative/iterative\\" }
         }
       }
-
       provider \\"iterative\\" {}
 
-
       resource \\"iterative_cml_runner\\" \\"runner\\" {
         repo = \\"https://\\"
         token = \\"abc\\"