specify TPI version

[casperdcl]

• Specify minimum terraform-provider-iterative version (may fix edge cases)
• follow-up to Deploy runner on EC2 gives error #883

[0x2b3bfa0]

🤔 What edge cases is this change trying to cover?

[casperdcl]

Things related to iterative/terraform-provider-iterative#266?

[DavidGOrtega]

@casperdcl if I do not specify the version I always get the latest.
I do not see why this PR is needed.
Could you please elaborate a bit more?

[dacbd]

could be nice, in combo with: iterative/terraform-provider-iterative#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 💯

[DavidGOrtega]

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" }

[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 💥 xD

[dacbd]

my brain auto translated "specify TPI version" to "Allow user to set 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. Deploy runner on EC2 gives error #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 they 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 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 iterative/terraform-provider-iterative#103 should suffice to close the stange loop between “TPI” and CML.

[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 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 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:

cml/bin/cml/runner.js

Lines 375 to 383 in 01b2cef

	const tpl = tf.iterativeProviderTpl();
	await fs.writeFile(tfMainPath, tpl);
	await tf.init({ dir: tfPath });
	await tf.apply({ dir: tfPath });
	const path = join(tfPath, 'terraform.tfstate');
	const tfstate = await tf.loadTfState({ path });
	tfstate.resources = [
	JSON.parse(Buffer.from(tfResource, 'base64').toString('utf-8'))
	];

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]

I think I'm on the side of (for lack of better words); it doesn't matter; the only action performed is the delete. The more critical component behind this idea was that --cml-version regarding the created instance side.

Happy to revisit if people think otherwise.

[0x2b3bfa0]

Static review: looks good to me!

[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:b4b648188a85c77c7285247d9830cf6ce0ac4c67415dc4e6b0794f3d7c950ad7",
    "zh:cc5624f08b9b1e4514fac5b324c150713c2b9e63c7f3874f37792e125ca72aed",
    "zh:e0707971f3cf1c0e011290e6e76a05b03183f756c91b5b9bd9c5c5027c8b2353",
    "zh:e5bb7ad0fae6d50ca04109cfaa57ef47a263fd6eb00eec51610623fecbb06c5e",
  ]
}
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 = []
}