feat: allow to show required by module version explicitly #301

ikorchynskyi · 2023-04-20T14:52:12Z

Proposed changes simplify the possibility to use tfswitch in automation: my own use-case is to fetch the required by project terraform version and to use specific version of docker image with terraform inside the CI/CD pipeline.

Related diagnostic messages has been moved to stderr to allow output to be fetched like

$ echo "Version required by module: $(go run main.go --chdir ~/foo/ --show-latest-required)"
Reading required version from terraform file
Reading required version from constraint: ~> 1.4.0, < 1.4.5
Matched version: 1.4.4
Version required by module: 1.4.4

yermulnik · 2023-04-21T11:16:36Z

Somewhat alike use case that I created issue two years ago for: #154

yermulnik

This feature might probably need to be reflected in README
New cmdline arg should be covered with test(s) if possible please
This works oddly on my end:

> git branch
  master
* pr/301

> git diff
diff --git a/main.go b/main.go
index 614e572..286be1c 100644
--- a/main.go
+++ b/main.go
@@ -455,8 +455,9 @@ func installOption(listAll bool, custBinPath, mirrorURL *string) {
 // install when tf file is provided
 func installTFProvidedModule(dir string, custBinPath, mirrorURL *string) {
        fmt.Println("Reading required version from terraform file")
+       fmt.Printf("%s\n", dir)
        module, _ := tfconfig.LoadModule(dir)
-       if len(module.RequiredCore) >= 1 {
+       if len(module.RequiredCore) == 0 {
                fmt.Println("The provided directory does not have required terraform version constraint.")
                os.Exit(1)
        }

> go run main.go --chdir /some/path/ --show-latest-required
Reading configuration from home directory for .tfswitch.toml
Reading required version from terraform file /some/path/
Reading required version from constraint: ~> 1.1.0
Matched version: 1.1.9
Switched terraform to version "1.1.9"

> terraform version | head -1
Terraform v1.1.9

> go run main.go --chdir /some/path/ --show-latest-required
Reading configuration from home directory for .tfswitch.toml
Reading required version from terraform file /some/path/
Reading required version from constraint: ~> 1.1.0
Matched version: 1.1.9
Switched terraform to version "1.1.9"

I would not expect this new feature to switch TF version for me.

yermulnik · 2023-04-21T11:28:48Z

lib/semver.go

@@ -12,7 +13,7 @@ func GetSemver(tfconstraint *string, mirrorURL *string) (string, error) {

 	listAll := true
 	tflist, _ := GetTFList(*mirrorURL, listAll) //get list of versions
-	fmt.Printf("Reading required version from constraint: %s\n", *tfconstraint)
+	fmt.Fprintf(os.Stderr, "Reading required version from constraint: %s\n", *tfconstraint)


I think change from use of fmt.Printf() to fmt.Fprintf(os.Stderr, … should not be applied as an ad-hoc and partially, but the whole tfswitch codebase should be revisited to see where fmt.Fprintf(os.Stderr, … is going to be more appropriate.
Instead of switching to fmt.Fprintf(os.Stderr, … for this particular use case, I'd suggest to have a cmdline flag created to provide verbose or terse (and even JSON) output, so that the new feature from this PR can be used programmatically w/o it printing human-friendly messages.

The reason behind that changes for me is that without output separation the tool is unusable in automation (I am not taking into account the "parse output" approach for obvious reasons).

My point is that such a change should be applied for all code instead of just a bit of it.
As for your reasoning (it all makes sense!) this is why I suggested to add a command line flag to "turn on" or "turn off" verbosity ("verbosity" as in printing to stdout).

@warrensbox @jukie What do you think?

tbh, I'd move from the printf kind of logging to some dedicated solution, like e.g. zerolog. Makes handling much easier - and allows to tweak verbosity by design.
I may prepare the appropriate change if needed

I may prepare the appropriate change if needed

That would be great if you feel like doing that. Contributions are very welcome.
Would be great if you got zerolog'ing implemented with pretty logging turned on by default (since the most audience for tfswitch are humans) with an option to turn it off (as in use default of JSON output format) for convenience in CI/CD.
Thank you.

@yermulnik I will wait till the @warrensbox response, as I do not have a clear vision in terms of a direction the tool suppose to move :)
My interest here is to make the CI/CD ready solution which may help with the following flow:

We have a terraform project

We do something to get the terraform version that would be able to make init, plan and apply with as little configuration and duplication as possible (having required_version and some specific version in .terraform-version is still treated a duplication)

We use that exact version to pull the specific tag of the official Docker image with terraform to execute the p. 2

As of now - I've come with a https://github.com/ikorchynskyi/terraform-version-inspect as a solution - the part of version constraint functionality from tfswitch and some stricter parsing using json release version "API" w/o relying on regex parsing

As of now - I've come with a https://github.com/ikorchynskyi/terraform-version-inspect as a solution

👍🏻 I already built it locally and will see if I'll have a chance to leverage it =)

The list of available versions is taken from https://releases.hashicorp.com/.

FYI: it's been already a year since Hashicorp announces API for that — https://www.hashicorp.com/blog/announcing-the-hashicorp-releases-api

Yeah, using the real API would be even better (instead of full index.json, so it is likely the plan for v0.2 :)

so it is likely the plan for v0.2 :)

Sounds great. This is how OSS comes into the world =)
Pre-built binaries and Homebrew tap would also be kewl to have 😺

Yes, will add.
I will leave this PR for further discussion with maintainers, but as of now - I've updated terraform-version-inspect to use releases API, and it seems that it serves our needs quite well - already integrated in CI/CD pipelines and works as expected

yermulnik · 2023-04-21T11:31:00Z

main.go

@@ -60,6 +60,8 @@ func main() {
 	showLatestStable := getopt.StringLong("show-latest-stable", 'S', defaultLatest, "Show latest implicit version. Ex: tfswitch --show-latest-stable 0.13 prints 0.13.7 (latest)")
 	latestFlag := getopt.BoolLong("latest", 'u', "Get latest stable version")
 	showLatestFlag := getopt.BoolLong("show-latest", 'U', "Show latest stable version")
+	latestRequiredFlag := getopt.BoolLong("latest-required", 'r', "Get latest required by module version")


What's this argument use case? Ain't this the default behavior of tfswitch when no cmdline args are provided?

The use case is - to install precisely from the module, not from the other probably available source. Same as with other "latest-*" flags - when they are provided, only that part/way of version resolution should be taken into account

Aha, got it. So it's sort of an enforcement/explicitness of what does case checkTFModuleFileExist(*chDirPath) && len(args) == 0: conditional. Makes sense.
Should probably use installTFProvidedModule(*chDirPath, &binPath, mirrorURL) then (prefix binPath with &) at line 125 in main.go in this PR.

prefix binPath with &

Well, custBinPath is already a pointer to string, and binPath is just the dereferenced value, so they are the same:

case fileExists(TOMLConfigFile) || fileExists(HomeTOMLConfigFile): version := "" binPath := *custBinPath

My point is about consistency across the same codebits.

Just like mine :)
custBinPath is used in the surrounding code in other case constructions in this part - so I've used the same style precisely

Aha, I got your point. We're talking about different "consistencies" =) I'm basing mine off of the use of installTFProvidedModule() for checkTFModuleFileExist() case within the same case fileExists(TOMLConfigFile) || fileExists(HomeTOMLConfigFile) block.

Though... it looks like binPath vs custBinPath logic is messed up and is a bit convoluted within the case fileExists(TOMLConfigFile) || fileExists(HomeTOMLConfigFile): conditional. Especially given the comment that custBinPath should override TOML config value.
So seemingly the logic intended should've been something like if custBinPath == defaultBin; then read TOML config; else binPath = custBinPath, but it is vice versa and hence some nested conditional cases use custBinPath, while others use bibPath 😲 Or I'm missing something essential… 🤔

@ikorchynskyi @jukie @warrensbox What do you guys think of last paragraph above ⬆️ re the intended logic — should we rectify this or it's all fine and @ikorchynskyi should keep using custBinPath for latestRequiredFlag case (see blow)?

terraform-switcher/main.go

Lines 123 to 125 in cb690d5

/* latest version required by module */

case *latestRequiredFlag:

installTFProvidedModule(*chDirPath, custBinPath, mirrorURL)

main.go

ikorchynskyi · 2023-04-21T12:41:06Z

This feature might probably need to be reflected in README

New cmdline arg should be covered with test(s) if possible please

This works oddly on my end:

But seems any "--show-*" flag works in a same way when there is a .toml file provided

	case fileExists(TOMLConfigFile) || fileExists(HomeTOMLConfigFile):

in that case there is no --show flag check at all.

yermulnik · 2023-04-23T19:42:22Z

This works oddly on my end:

But seems any "--show-*" flag works in a same way when there is a .toml file provided
	case fileExists(TOMLConfigFile) || fileExists(HomeTOMLConfigFile):
in that case there is no --show flag check at all.

Oh, good catch. I missed this behavior =( Could you probably file an issue for this to get fixed (so that --show* flags don't actually switch TF version)? Thanks.

@warrensbox @jukie Any clue on whether this was implemented this way on purpose or this is indeed a bug?

ikorchynskyi · 2023-04-24T11:15:26Z

Created #302 to clarify the flags logic

MatthewJohn · 2024-07-29T21:34:04Z

I know this is quite old, but we have a recent issue that @yermulnik thinks it will fix.
Is it worth updating to align with main? Is there still interest in this?

feat: allow to show required by module version explicitly

cb690d5

yermulnik reviewed Apr 21, 2023

View reviewed changes

fix: correct RequiredCore check in installTFProvidedModule

8348f71

ikorchynskyi added 2 commits April 24, 2023 13:00

fix: return back skipping the duplicated definitions

a54a9c0

fix: resolve staticcheck warnings

78eeb41

yermulnik linked an issue Jul 5, 2024 that may be closed by this pull request

TFSwitch --latest should have an equivalent that will conform to version constraints in tf files #475

Open

yermulnik added the new feature New feature or request label Jul 5, 2024

yermulnik mentioned this pull request Jul 5, 2024

TFSwitch --latest should have an equivalent that will conform to version constraints in tf files #475

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: allow to show required by module version explicitly #301

feat: allow to show required by module version explicitly #301

ikorchynskyi commented Apr 20, 2023

yermulnik commented Apr 21, 2023

yermulnik left a comment

yermulnik Apr 21, 2023

ikorchynskyi Apr 21, 2023

yermulnik Apr 23, 2023

ikorchynskyi Apr 24, 2023

yermulnik Apr 24, 2023

ikorchynskyi May 2, 2023

yermulnik May 2, 2023 •

edited

Loading

ikorchynskyi May 2, 2023

yermulnik May 2, 2023

ikorchynskyi May 3, 2023

yermulnik Apr 21, 2023

ikorchynskyi Apr 21, 2023

yermulnik Apr 23, 2023

ikorchynskyi Apr 24, 2023

yermulnik Apr 24, 2023

ikorchynskyi Apr 24, 2023

yermulnik Apr 24, 2023

ikorchynskyi commented Apr 21, 2023

yermulnik commented Apr 23, 2023

ikorchynskyi commented Apr 24, 2023

MatthewJohn commented Jul 29, 2024

	/* latest version required by module */
	case *latestRequiredFlag:
	installTFProvidedModule(*chDirPath, custBinPath, mirrorURL)

feat: allow to show required by module version explicitly #301

Are you sure you want to change the base?

feat: allow to show required by module version explicitly #301

Conversation

ikorchynskyi commented Apr 20, 2023

yermulnik commented Apr 21, 2023

yermulnik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yermulnik May 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ikorchynskyi commented Apr 21, 2023

yermulnik commented Apr 23, 2023

ikorchynskyi commented Apr 24, 2023

MatthewJohn commented Jul 29, 2024

yermulnik May 2, 2023 •

edited

Loading