Check version sources based on whether verison has been set #429

MatthewJohn · 2024-05-09T06:36:39Z

Update logic for reading sources for obtaining version to read from next source explicitly if version has not been set. This handles situations where a source exists (e.g. TOML file, terraform files), but a version constraint cannot be found from the source. This avoids duplicate checks for existance of sources (such as Terragrunt file), as this was performed in GetParamters and also in the methods to read the config from the source.

Issue #420

Update logic for reading sources for obtaining version to read from next source explicitly if version has not been set. This handles situations where a source exists (e.g. TOML file, terraform files), but a version constraint cannot be found from the source. This avoids duplicate checks for existance of sources (such as Terragrunt file), as this was performed in GetParamters and also in the methods to read the config from the source. Issue #420

…ParamsTOML also checks if the TOML file exists Issue #420

…sion, as these are handled directly from the GetVersionFromX methods Issue #420

…ic errors This was showing a warning as diagnostics from gohcl.DecodeBody was not used. Issue #420

Issue #420

…uations where no version is available when config files are present Issue #420

lib/param_parsing/parameters.go

…form_version, as these are handled directly from the GetVersionFromX methods" This reverts commit 90433e0.

…arameters from them Issue #420

MatthewJohn · 2024-05-10T05:54:06Z

I initially started looking at the, now, failing tests...
The first one I spotted was due to the TOML file failing to override the env variable for version - this is obviously testing the opposite of what was agreed in #420 (env variables have highest priority), so I'm going to leave these failing for now to get some feedback on the suggestion :)

lib/param_parsing/parameters.go

warrensbox · 2024-05-13T09:30:34Z

Still reviewing...

lib/param_parsing/toml.go

lib/param_parsing/parameters.go

Issue #420

… overriding params Version with environment variable

…void polluting other tests

Previously, the highest priority was the TOML file, however, the new ordering means that terragrunt files have the highest precedence: #420

…suring each configuring overrides the previous

…y no longer contains any indicators of a valid version Issue #420

lib/param_parsing/parameters.go

lib/param_parsing/parameters_test.go

…on files Issue #420

…tion permissions, avoiding other read/write Issue #420

Only call GetTomlParams once and update methods to unify getting base directory Issue #420

Update getTomlParams to use TOML directory attribute of Params Update tests to set TOML directory to set custom location for test TOML file. Issue #420

…ams methods. This allowstests to inject custom default params, by calling the two child methods seperately, instead of calling GetParameters Issue #420

…ctory, if it exists

…ing symlinks

lib/param_parsing/versiontf.go

Co-authored-by: George L. Yermulnik <yz@yz.kiev.ua>

yermulnik · 2024-06-04T16:05:35Z

.github/workflows/build.yml

+            if test -f "${TEST_PATH}/.tfswitch.toml"
+            then
+                cp "${TEST_PATH}/.tfswitch.toml" ~/
+            else
+                { test -f ~/.tfswitch.toml && rm ~/.tfswitch.toml; } || true
+            fi


What this if-conditional's goal is?
If test TOML exists, then copy it to HOME, otherwise delete TOML from HOME 😲

Exactly that - it's handled the fact that a test has a TOML file, but we ignore ChDiR (CWD) now, but I wanted to keep the test... so copied to HOMEDIR so it continued to work

Ah, I lean to think I got it =)
You should be good to replace { test -f ~/.tfswitch.toml && rm ~/.tfswitch.toml; } || true with a simpler rm -f ~/.tfswitch.toml

yermulnik · 2024-06-04T16:14:48Z

lib/param_parsing/parameters.go

+			logger.Fatalf("Failed to obtain settings from TOML config in home directory: %v", err)
+		}
+	}
+
+	if tfSwitchFileExists(params) {
 		params, err = GetParamsFromTfSwitch(params)
-	} else if terraformVersionFileExists(params) {
+		if err != nil {
+			logger.Fatalf("Failed to obtain settings from \".tfswitch\" file: %v", err)
+		}
+	}
+
+	if terraformVersionFileExists(params) {
 		params, err = GetParamsFromTerraformVersion(params)
-	} else if isTerraformModule(params) {
+		if err != nil {
+			logger.Fatalf("Failed to obtain settings from \".terraform-version\" file: %v", err)
+		}
+	}
+
+	if isTerraformModule(params) {
 		params, err = GetVersionFromVersionsTF(params)
-	} else if terraGruntFileExists(params) {
-		params, err = GetVersionFromTerragrunt(params)
-	} else {
-		params = GetParamsFromEnvironment(params)
+		if err != nil {
+			logger.Fatalf("Failed to obtain settings from Terraform module: %v", err)
+		}
 	}
-	if err != nil {
-		logger.Fatalf("Error parsing configuration file: %q", err)
+
+	if terraGruntFileExists(params) {
+		params, err = GetVersionFromTerragrunt(params)
+		if err != nil {
+			logger.Fatalf("Failed to obtain settings from Terragrunt configuration: %v", err)


Instead of hardcoding and wording version sources, could we probably move those from lib/param_parsing/*.go files to a more global place and re-use?

MatthewJohn requested review from warrensbox, yermulnik, jukie, MatrixCrawler and crablab as code owners May 9, 2024 06:36

MatthewJohn added 6 commits May 9, 2024 07:37

Remove check if TOML file exists before calling getParamsTOML, as get…

7df2b96

…ParamsTOML also checks if the TOML file exists Issue #420

Remove unused checkXExists methods in toml, terragrunt, terraform_ver…

90433e0

…sion, as these are handled directly from the GetVersionFromX methods Issue #420

Add check for ensuring that gohcl.DecodeBody does not return diagnost…

b1467d4

…ic errors This was showing a warning as diagnostics from gohcl.DecodeBody was not used. Issue #420

Ignore diagnostics from gohcl.DecodeBody to retain original behavoir

104d48d

Update version param_parsing tests to check err return values

b1db9e2

Issue #420

Check version value after error from TOML file reading

627ef90

Issue #420

MatthewJohn force-pushed the 420-how-to-deal-with-toml-file branch from 383fff9 to 627ef90 Compare May 9, 2024 07:00

Add tests to ensure terragrunt, TOML and terraform version handle sit…

9b95ed7

…uations where no version is available when config files are present Issue #420

MatthewJohn marked this pull request as draft May 9, 2024 07:03

MatthewJohn changed the title ~~DRAFT: Check version sources based on whether verison has been set~~ Check version sources based on whether verison has been set May 9, 2024

MatthewJohn commented May 9, 2024

View reviewed changes

lib/param_parsing/parameters.go Outdated Show resolved Hide resolved

MatthewJohn added 2 commits May 9, 2024 17:03

Revert "Remove unused checkXExists methods in toml, terragrunt, terra…

3a0973d

…form_version, as these are handled directly from the GetVersionFromX methods" This reverts commit 90433e0.

Re-introduce checking if configuration types exist before obtaining p…

9fe6973

…arameters from them Issue #420

MatthewJohn commented May 10, 2024

View reviewed changes

lib/param_parsing/parameters.go Outdated Show resolved Hide resolved

warrensbox reviewed May 24, 2024

View reviewed changes

lib/param_parsing/toml.go Outdated Show resolved Hide resolved

warrensbox reviewed May 24, 2024

View reviewed changes

lib/param_parsing/parameters.go Outdated Show resolved Hide resolved

warrensbox reviewed May 24, 2024

View reviewed changes

lib/param_parsing/parameters.go Outdated Show resolved Hide resolved

MatthewJohn added 5 commits May 26, 2024 05:26

Remove unused parameter 'params' in 'tomlFileExists'

c3224c9

Issue #420

Ensure environment variable has been set to a non-empty string before…

ea41eca

… overriding params Version with environment variable

Unset environment variable for TF_VERSION in tests after setting to a…

8c22bca

…void polluting other tests

Update expected value in precedence to new expected value.

25d1953

Previously, the highest priority was the TOML file, however, the new ordering means that terragrunt files have the highest precedence: #420

Update precendence test to create each configuration incrementally en…

58f3a49

…suring each configuring overrides the previous

MatthewJohn mentioned this pull request May 26, 2024

Reading automatically from ~/.tfswitch.toml is broken #417

Closed

test: Move precedence check to skip-integration-tests as the director…

0ef7b9b

…y no longer contains any indicators of a valid version Issue #420

MatthewJohn force-pushed the 420-how-to-deal-with-toml-file branch from 36ebed9 to 0ef7b9b Compare May 26, 2024 11:36

yermulnik reviewed May 26, 2024

View reviewed changes

MatthewJohn added 2 commits May 27, 2024 06:00

Improve log messages when unable to read from third party configurati…

8437e8f

…on files Issue #420

Update permissions of test files created by tests to use more restric…

8e5e83f

…tion permissions, avoiding other read/write Issue #420

This was linked to issues May 31, 2024

Version: v1.1.1 no longer reads terraform required_version anymore #441

Closed

Reading automatically from ~/.tfswitch.toml is broken #417

Closed

log-level TOML config doesn't work #421

Closed

yermulnik mentioned this pull request May 31, 2024

log-level TOML config doesn't work #421

Closed

warrensbox mentioned this pull request Jun 1, 2024

OpenTofu support and JSON recentFile support #435

Merged

warrensbox added this to the Release 1.2.0 milestone Jun 1, 2024

MatthewJohn added 3 commits June 3, 2024 07:02

fix: Only obtain TOML file from home directory

f7c7032

Only call GetTomlParams once and update methods to unify getting base directory Issue #420

Move TOML directory to attribute of Params, defaulting to HomeDir.

161e0d5

Update getTomlParams to use TOML directory attribute of Params Update tests to set TOML directory to set custom location for test TOML file. Issue #420

Split GetParameters logic to just call initParams and new populatePar…

8555500

…ams methods. This allowstests to inject custom default params, by calling the two child methods seperately, instead of calling GetParameters Issue #420

MatthewJohn force-pushed the 420-how-to-deal-with-toml-file branch from 9ccdee9 to b9c3259 Compare June 3, 2024 06:45

Update integration tests to symlink tfswitch TOML in CWD to home dire…

3630353

…ctory, if it exists

MatthewJohn force-pushed the 420-how-to-deal-with-toml-file branch from b9c3259 to 3630353 Compare June 3, 2024 06:45

Update build to copy .tfswitch.toml to home directory, rather than us…

21400f9

…ing symlinks

MatthewJohn mentioned this pull request Jun 3, 2024

Version: v1.1.1 no longer reads terraform required_version anymore #441

Closed

Add warning when failing to parse Terraform

b2a5916

MatthewJohn mentioned this pull request Jun 4, 2024

Standardise logic for failing/warning when an error occurs during terraform version detection #451

Open

MatthewJohn requested review from warrensbox and yermulnik June 4, 2024 15:48

yermulnik reviewed Jun 4, 2024

View reviewed changes

lib/param_parsing/versiontf.go Outdated Show resolved Hide resolved

Improve error in warning for invalid Terraform module

309b0ed

Co-authored-by: George L. Yermulnik <yz@yz.kiev.ua>

yermulnik reviewed Jun 4, 2024

View reviewed changes

Merge branch 'master' into 420-how-to-deal-with-toml-file

17507bf

warrensbox merged commit fc5cd6b into master Jun 7, 2024
3 checks passed

warrensbox deleted the 420-how-to-deal-with-toml-file branch June 7, 2024 21:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check version sources based on whether verison has been set #429

Check version sources based on whether verison has been set #429

MatthewJohn commented May 9, 2024

MatthewJohn commented May 10, 2024

warrensbox commented May 13, 2024

yermulnik Jun 4, 2024 •

edited

Loading

MatthewJohn Jun 4, 2024

yermulnik Jun 4, 2024

yermulnik Jun 4, 2024

Check version sources based on whether verison has been set #429

Check version sources based on whether verison has been set #429

Conversation

MatthewJohn commented May 9, 2024

MatthewJohn commented May 10, 2024

warrensbox commented May 13, 2024

yermulnik Jun 4, 2024 • edited Loading

Choose a reason for hiding this comment

MatthewJohn Jun 4, 2024

Choose a reason for hiding this comment

yermulnik Jun 4, 2024

Choose a reason for hiding this comment

yermulnik Jun 4, 2024

Choose a reason for hiding this comment

yermulnik Jun 4, 2024 •

edited

Loading