The toml file - How to deal with it.

[warrensbox]

The TOML file was created because users wanted to specify a bin path location in a config file. When we first released this feature, the toml file was in the HOME directory. So when a user uses tfswitch in any directory, it immediately knows where to install the bin path.

~/home
-- .tfswitch.toml (ex bin.path = /usr/local/warrenv/.bin)

User can run tfswitch anywhere not having to worry where the bin path would be. They do not need to pass the -b parameter all the time, as well.

What if you just wanted to have the version also configured in the toml file? That's were the version option came in. But again this can be overridden later.

But what is the .toml file is not in the HOME directory. That's where the option to pass the -c came in.

SO we still need to make the toml file readable as well incorporating the ability to have the version overridden.

see: https://github.com/warrensbox/terraform-switcher/blob/1.0.2/main.go#L99C2-L99C68

case fileExists(TOMLConfigFile) || fileExists(HomeTOMLConfigFile):
		version := ""
		binPath := *custBinPath
		if fileExists(TOMLConfigFile) { //read from toml from current directory
			version, binPath = getParamsTOML(binPath, *chDirPath)
		} else { // else read from toml from home directory
			version, binPath = getParamsTOML(binPath, homedir)
		}

		switch {
		/* GIVEN A TOML FILE, */
		/* show all terraform version including betas and RCs*/
		case *listAllFlag:
			listAll := true //set list all true - all versions including beta and rc will be displayed
			installOption(listAll, &binPath, mirrorURL)
		/* latest pre-release implicit version. Ex: tfswitch --latest-pre 0.13 downloads 0.13.0-rc1 (latest) */
		case *latestPre != "":
			preRelease := true
			installLatestImplicitVersion(*latestPre, &binPath, mirrorURL, preRelease)
		/* latest implicit version. Ex: tfswitch --latest 0.13 downloads 0.13.5 (latest) */
		case *latestStable != "":
			preRelease := false
			installLatestImplicitVersion(*latestStable, &binPath, mirrorURL, preRelease)
		/* latest stable version */
		case *latestFlag:
			installLatestVersion(&binPath, mirrorURL)
		/* version provided on command line as arg */
		case len(args) == 1:
			installVersion(args[0], &binPath, mirrorURL)
		/* provide an tfswitchrc file (IN ADDITION TO A TOML FILE) */
		case fileExists(RCFile) && len(args) == 0:
			readingFileMsg(rcFilename)
			tfversion := retrieveFileContents(RCFile)
			installVersion(tfversion, &binPath, mirrorURL)
		/* if .terraform-version file found (IN ADDITION TO A TOML FILE) */
		case fileExists(TFVersionFile) && len(args) == 0:
			readingFileMsg(tfvFilename)
			tfversion := retrieveFileContents(TFVersionFile)
			installVersion(tfversion, &binPath, mirrorURL)
		/* if versions.tf file found (IN ADDITION TO A TOML FILE) */
		case checkTFModuleFileExist(*chDirPath) && len(args) == 0:
			installTFProvidedModule(*chDirPath, &binPath, mirrorURL)
		/* if Terraform Version environment variable is set */
		case checkTFEnvExist() && len(args) == 0 && version == "":
			tfversion := os.Getenv("TF_VERSION")
			fmt.Printf("Terraform version environment variable: %s\n", tfversion)
			installVersion(tfversion, &binPath, mirrorURL)
		/* if terragrunt.hcl file found (IN ADDITION TO A TOML FILE) */
		case fileExists(TGHACLFile) && checkVersionDefinedHCL(&TGHACLFile) && len(args) == 0:
			installTGHclFile(&TGHACLFile, &binPath, mirrorURL)
		// if no arg is provided - but toml file is provided
		case version != "":
			installVersion(version, &binPath, mirrorURL)
		default:
			listAll := false //set list all false - only official release will be displayed
			installOption(listAll, &binPath, mirrorURL)
		}

Please let me know if you have any questions. We should refactor the existing code to match this behavior. I am open to other ideas as well.

@yermulnik @MatrixCrawler @crablab @MatthewJohn

[yermulnik]

I agree. This all makes sense. But this means we need to reverse the order of precedence in the docu as the highest precedence actually has env var, right?

• TOML in home dir defines version if file is present and if version is present
  • also defines other parameters like bin and log-level
• TOML in home dir is not taken into account if TOML in current dir exists and TOML in current dir defines version and parameters
  • TOML in current dir is not taken into account if -c is provided
    • TOML in -c defines parameters if file is present
• .tfswitchrc in current dir overrides version from previous items if file is present
  • .tfswitchrc in current dir is not taken into account if .tfswitchrc in -c dir is present and .tfswitchrc in -c dir overrides version from previous items if file is present
• Same applies for .terraform-version
• Terraform root module in current dir overrides version if required_version is present
  • Is not taken into account if -c is provided which overrides version if required_version is present
• Same for terragrunt.hcl
• env var TF_VERSION overrides all the previous if defined

Caveat: non-version parameters can be set in TOM file only and the TOML from current dir (or from -c dir) takes precedence over the one from home dir.

Does this sound meaningful?

[MatthewJohn]

The only thing that makes me.. concerned about this approach is that:

• TOML file defaults to HOMEDIR
• "code directory" (containing .terraform-version etc. etc.) is in PWD

Both are overridden by the same -c.
This could cause an ambiguity between saying: I'm overriding the TOML file directory (find the TOML file in -c) and I'm overriding the TF directory (find the tfversion etc. files in there).
If we're just talking about picking up the param for Version, this might make sense.. but we're not, there's other configs in the TOML.

So, referencing back to you list above @yermulnik , we're not taking the TOML file in the HOMEDIR into account..

That's all to say, is it that there's just a missing bullet point of " * TOML in HOMEDIR" at the end and the -c flag only diferenciates between CWD and -c dir?

[yermulnik]

Hmm, you pushed me to think that I might have messed up things a bit and "overengineered" the TOML bit. Based on #420 (comment) TOML should not be the scope of -c arg, but CWD was taken into account previously though.

On the other hand, you're right that having TOML in CWD or in -c dir introduces ambiguity and complexity, especially when debugging issues. Though... though having -c and CWD considered for TOML to define custom settings (not override settings from TOML in HOME, but define settings independently and exclusively) might make sense for edge cases where a user may want to have custom configuration for a particular codebase... 🤔
This though may be a real overengineering as vast majority of tfswitch use cases is about TF version only and TOML is there to only simplify usage of a few cmdline args.
Given the above, what do you think is better: allow only TOML from HOME or additionally allow TOML in CWD and -c? (I'd personally prefer the former — the only and single TOML in HOME)

ℹ️ I updated my previous comment by dropping CWD and -c for TOML.

Let me know if I misunderstood you point though.

ps: the other topic might be whether we want to respect XDG spec (https://wiki.gentoo.org/wiki/XDG/Base_Directories) for config file location (probably additionally to the "default" of looking it up from $HOME directly) 🤷🏻

[MatthewJohn]

I've added a draft PR (afraid I'm feeling very "cold"-ey this evening so can't really concentrate).. but hopefully gives an idea in the direction I'm thinking about suggesting: #429

[yermulnik]

👍🏻 I saw the notification but left it for later as I've been pretty busy doing work stuff lately. Will do my best to review as soon as possible. Hope others will do the same.

[yermulnik]

Adjusted docu: #492

[MatthewJohn]

So, yes, the main issue I'm seeing, assuming I've got the right impression is this: https://github.com/warrensbox/terraform-switcher/blob/master/lib/param_parsing/parameters.go#L59

	if tomlFileExists(params) {
		params, err = getParamsTOML(params)
	} else if tfSwitchFileExists(params) {
		params, err = GetParamsFromTfSwitch(params)
	} else if terraformVersionFileExists(params) {
		params, err = GetParamsFromTerraformVersion(params)
	} else if isTerraformModule(params) {
		params, err = GetVersionFromVersionsTF(params)
	} else if terraGruntFileExists(params) {
		params, err = GetVersionFromTerragrunt(params)
	} else {
		params = GetParamsFromEnvironment(params)
	}
	if err != nil {
		logger.Fatalf("Error parsing configuration file: %q", err)
	}

If a TOML file exists (whether it specifies the version or no)t, then we're just completely ignoring versions from any other sources

[warrensbox]

Yeah, this logic is incorrect. In the previous version, if you had a toml file, the version can be overridden.

Would you like to take a stab at this?
@MatthewJohn

[MatthewJohn]

Sure :) Happy to have a go :)

[yermulnik]

Issues:

• Reading automatically from ~/.tfswitch.toml is broken #417
• tfswitch ignored -b argument #427 (looks like unrelated)

[warrensbox]

@MatthewJohn @yermulnik
We have the TOML discussion all over the place. Lets finalize our decision in this discussion.

[warrensbox]

This bug has been out for awhile and nobody has complained a lot. Perhaps, people are not using the TOML file as much as we thought they would.

I say lets have TOML in the home directory for now (only). If someone opens an issue, we know that TOML is used beyond the home directory.

@MatthewJohn please proceed with these changes. Otherwise your code looks pretty solid

[MatthewJohn]

All good from my side nice - happy for any final reviews :)

[warrensbox]

@MatthewJohn can you please fix this bug as well?

#443

It's related to the parameters.

[yermulnik]

I added some thoughts to #443

[MatthewJohn]

And you've fixed it as well :) 🙏

[warrensbox]

@yermulnik can you please help me test these 2 cases:

1. Download binary tfswitch 1.0.2
2. Create a version.tf file

terraform {
  required_version = "1.6.3"
}

3. Run tfswitch --latest (1.0.2).

It should change the terraform version to 1.8.4 not 1.6.3

However the latest version of tfswitch does not do that. Can you please verify?

[yermulnik]

> ./zzz/tfswitch --version

Version: 1.0.2

> fgrep required_version *.tf
main.tf:  required_version = "1.6.3"

> ./zzz/tfswitch --latest
Reading configuration from home directory for .tfswitch.toml
Downloading to: /home/giermulnik/.terraform.versions
26745153 bytes downloaded
Switched terraform to version "1.8.4"

> tfswitch --version
13:59:50.491 INFO Reading version from Terraform module at "."
13:59:50.612 INFO Reading required version from constraint: "1.6.3"
13:59:50.615 INFO Matched version: "1.6.3"
Version: v1.1.1

> tfswitch --latest
13:59:55.015 INFO Reading version from Terraform module at "."
13:59:55.134 INFO Reading required version from constraint: "1.6.3"
13:59:55.137 INFO Matched version: "1.6.3"
13:59:55.159 INFO Installing terraform at "/home/giermulnik/bin"
13:59:55.201 INFO Switched terraform to version "1.8.4"

They behave the same for me. Did I get what you wanted me to do right?

The only caveat is related to #443 — it pointlessly reads TF version.

[yermulnik]

The only caveat is related to #443 — it pointlessly reads TF version.

Ref: #443 (comment)

[MatthewJohn]

Yes, from the code, I can't see how it would do anything other than install the latest version, since:

	case parameters.LatestFlag:
		/* latest stable version */
		lib.InstallLatestVersion(parameters.DryRun, parameters.CustomBinaryPath, parameters.InstallPath, parameters.MirrorURL)

the method doesn't get given parameters.Version :)

[warrensbox]

My bad. Looks like it is working as expected

[warrensbox]

@MatthewJohn @yermulnik
I am seeing this:

It is causing the the installation to fail

[warrensbox]

I think the way *recent is being appended is incorrect. Please see how we did it in 1.0.2

[yermulnik]

<humor>
Double recent is the most preferred most used most neat recent 🤣
</humor>

[yermulnik]

Cc @MatrixCrawler

[MatthewJohn]

This is now fixed in 59548d1

[warrensbox]

Also, when you do not have a RECENT file, it is no longer creating a RECENT file.
When a user first downloads tfswitch, they do not have a RECENT file. Or when someone accidentally deleted their RECENT file:

panic: runtime error: slice bounds out of range [:1] with capacity 0

goroutine 1 [running]:
github.com/warrensbox/terraform-switcher/lib.unmarshalRecentFileData({0x140000cc480, 0x29}, 0x1400021ca50)
	/Users/warrenv/WarrensBox/go/src/github.com/warrensbox/tfswitch/lib/recent.go:66 +0x1cc
github.com/warrensbox/terraform-switcher/lib.getRecentVersions({0x1400002a095?, 0x140000be360?}, {0x104e8e6b8, 0x1052135e0})
	/Users/warrenv/WarrensBox/go/src/github.com/warrensbox/tfswitch/lib/recent.go:46 +0x88
github.com/warrensbox/terraform-switcher/lib.InstallProductOption({0x104e8e6b8, 0x1052135e0}, 0x0, 0x0, {0x104ca05b6, 0x18}, {0x1400002a095, 0xe}, {0x104cae1d0, 0x28})
	/Users/warrenv/WarrensBox/go/src/github.com/warrensbox/tfswitch/lib/install.go:295 +0x64
main.main()
	/Users/warrenv/WarrensBox/go/src/github.com/warrensbox/tfswitch/main.go:73 +0x1f4

[warrensbox]

To reproduce this, try removing the RECENT file

[yermulnik]

Cc @MatrixCrawler

[warrensbox]

In the previous version (tag: 0.13.1316) :

func installOption(listAll bool, custBinPath, mirrorURL *string) {
	tflist, _ := lib.GetTFList(*mirrorURL, listAll) //get list of versions
	recentVersions, _ := lib.GetRecentVersions()    //get recent versions from RECENT file
	tflist = append(recentVersions, tflist...)      //append recent versions to the top of the list
	tflist = lib.RemoveDuplicateVersions(tflist)    //remove duplicate version

	if len(tflist) == 0 {
		fmt.Println("[ERROR] : List is empty")
		os.Exit(1)
	}
	/* prompt user to select version of terraform */
	prompt := promptui.Select{
		Label: "Select Terraform version",
		Items: tflist,
	}

	_, tfversion, errPrompt := prompt.Run()
	tfversion = strings.Trim(tfversion, " *recent") //trim versions with the string " *recent" appended

	if errPrompt != nil {
		log.Printf("Prompt failed %v\n", errPrompt)
		os.Exit(1)
	}

	lib.Install(tfversion, *custBinPath, *mirrorURL)
	os.Exit(0)
}

I had tflist, _ := lib.GetTFList(*mirrorURL, listAll) //get list of versions, I try getting all the version first and then match it the recent version.

[MatthewJohn]

Added tests in f558e54 and fixed in c9c8272

[MatthewJohn]

I had changed it in the original PR to only append in the UI.. but after merging main into the branch with the Redone JSON changes, I missed re-removing the functionality

…

Sent from my iPhone

On 6 Jun 2024, at 21:23, Warren Veerasingam ***@***.***> wrote:  I think the way *recent is being appended is incorrect. Please see how we did it in 1.0.2 — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.

[warrensbox]

No worries, this is why we do reviews - to catch stuff like this.

[warrensbox]

Thanks @MatthewJohn, once you have fix the 2 issues above, please let me know. I are close to releasing this. Thank you for your work

[MatthewJohn]

@warrensbox both of those should be fixed :)

[prashantkalkar]

I agree to the environment variable being the highest precedence.
But I think the original order between .tfswitchrc and tf required_version precedence was correct (.tfswitchrc being higher precedence).
.tfswitchrc is very specific for the tfswitch choice of terraform version and should have higher precedence than the required_version from tf.

Please see this issue for a valid scenario for this: #491

[yermulnik]

It's been for years that the VERSION, RC, and TOML files were defining default (fallback) version if no other sources of versions were found. At some point in time during huge re-catoring we accidentally reversed the order of precedence and fixed it after that. It might be that you picked up "incorrect" behavior when it was in place and we do apologize for the inconveniences this brought into your environment.
At the moment the order is correct IMO.

[prashantkalkar]

@yermulnik Thank you for your reply and details. Despite the history of this change, I believe that .tfswitchrc being more specific and explicit about the choice of tfswitch tf version and hence should have higher precedence over the required_version from tf files.

The required_version can also represent version ranges which further decreases its specificity (this I think is not the case for .tfswitchrc which points to a specific version of the tf).

Just wanted to share my opinion as the user of the tool.

[MatthewJohn]

Hey,

Given that we now have the "default-version" parameter in env variables and TOML file, I would expect the order of precedence to be:

• env version (highest)
• toml version
• code (tfversion, terragrunt etc.)
• env default_version
• toml default_version (lowest)

What do you think?

Matt

[yermulnik]

The default version is of another kind I reckon and is what the --default arg is about (I hope) - it is not about a fallback version, it is about default version used when no other sources defined the required/desired version of TF

  -d, --default=value
                    Default to this version in case no other versions could be
                    detected. Ex: tfswitch --default 1.2.4

What we had discussed and agreed upon I see as a correct precedence order: #420 (comment)

I'm pretty sure we shouldn't be breaking existing logic that can be relied upon by someone else.
If Prashant's use case makes sense to someone else, then we need to implement new option which would take precedence right after env var and should probably be flexible rather than plain, as in provide TF version constraint rather than static version. Filename can be something like .terraform-version-override or something like that but more versatile and concise.

[MatthewJohn]

Ah apologies, I read through on my phone and confused the .tfswitchrc with TOML 🤦