Fix user bin path on windows

[PierreTechoueyres]

Allow use of bin keyword in toml config file

[yermulnik]

@PierreTechoueyres Thank you for the contribution. Would you mind elaborating on details of what and how this PR resolves? Thanks.

[PierreTechoueyres]

Without b86f96b8858aa1f9b65fa1968cf5536fea1d6a78 the binary is always installed in %USERPROFILE%\bin whatever you setup in your .tfswitch.toml.

For example I have an .tfswitch.toml like the one bellow

bin = "C:/Users/MyName/go/bin/terraform"
version = "1.8.4"

And without my patch, tfswitch always install the terraform binary in C:/Users/MyName/bin.

[PierreTechoueyres]

I think that my commit b86f96b8858aa1f9b65fa1968cf5536fea1d6a78 could be simplified.

-       binPathWritable := false //assume bin path is not writable
-       if runtime.GOOS != "windows" {
-           binPathWritable = CheckDirWritable(binDir) //check if is writable on ( only works on LINUX)
-       }
+       binPathWritable := runtime.GOOS == "windows" || CheckDirWritable(binDir) //check if is writable on ( only works on LINUX)

What do you think of it ?

[yermulnik]

Without b86f96b the binary is always installed in %USERPROFILE%\bin whatever you setup in your .tfswitch.toml.

This should be presumably fixed in the v1.2.0-alpha (and consequently in the nearest upcoming release): https://github.com/warrensbox/terraform-switcher/releases/tag/v1.2.0-alpha — can you please try this alpha and see whether bin value from TOML is respected correctly? Thanks.

[PierreTechoueyres]

I've already checked with latest mater (6b0b1901ddb37319b018dbce44d48f3943f3f2b1) and created my patch upon the code at it.

[yermulnik]

Ah, I see 👍🏻

• Please revert logging additions and changes as they partially overlap with #464 and partially add excessive logging I guess.
• Also do I get it right that you're suggesting to assume bin path on Windows is writable unconditionally (as opposite to existing logic that assumes Windows paths are not writable by default): https://github.com/warrensbox/terraform-switcher/pull/468/files#diff-90697d6eae2e2416dcb80680a07d02ed87ac625142983a3f4ce392f236f4be54R163 — did you have a chance to validate this change for paths outside your home directory on Windows? (unfortunately we have limited capacity to verify all of the changes on Windows 😢)
  • I suppose that the "custom bin path is not writable on Windows" bit was added on purpose and hence to allow custom bin paths on Windows we probably need to go a bit further and implement something like this https://stackoverflow.com/a/76285404/5093149 so that tfswitch doesn't just assume custom bin path is writable, but also ensures it is indeed writable before proceeding 🤔

Cc: @warrensbox @MatthewJohn @MatrixCrawler

[PierreTechoueyres]

* Please revert logging additions and changes as they partially overlap with [Add debug logging when successfully obtaining parameter from location… #464](https://github.com/warrensbox/terraform-switcher/pull/464) and partially add excessive logging I guess.

Done.

* Also do I get it right that you're suggesting to assume `bin` path on Windows is writable unconditionally (as opposite to existing logic that assumes Windows paths are not writable by default):  https://github.com/warrensbox/terraform-switcher/pull/468/files#diff-90697d6eae2e2416dcb80680a07d02ed87ac625142983a3f4ce392f236f4be54R163 — did you have  a chance to validate this change for paths outside your home directory on Windows? (unfortunately we have limited capacity to verify all of the changes on Windows 😢)

No, I didn't validate that a path outside the home directory on Windows was writable. But I think it's user responsibility to check he has the write access on this directory. I admit this isn't an ideal situation. But the other way is worse. Currently the option is simply ignored on Window, with this patch it let you a chance to use it.

  * I suppose that the "_custom bin path is not writable on Windows_" bit was added on purpose and hence to allow custom `bin` paths on Windows we probably need to go a bit further and implement something like this https://stackoverflow.com/a/76285404/5093149 so that `tfswitch` doesn't just assume custom `bin` path is writable, but also ensures it is indeed writable before proceeding 🤔

That's would definitely be a better way (and an OS agnostic one). I see there is already an implementation of CheckDirWritable in lib/dir_perm_windows.go. Do you know why it is discarded in current code ?

[PierreTechoueyres]

I've made an attempt with the informations from (https://stackoverflow.com/a/76285404/5093149) on branch PierreTechoueyres:fix-userBinPath-on-windows-1. With it and an unwritable path in my ~/.tfswitch.toml I obtain :

INFO Reading configuration from "C:\Users\MyName\.tfswitch.toml" ERROR Could not create file "tmpfile" on "C:\Program Files" INFO Installing Terraform at "C:\Users\MyName\bin" INFO Switched Terraform to version "1.8.4"

Whereas with the actual PR it shows

INFO Reading configuration from "C:\Users\MyName\.tfswitch.toml" INFO Found binary path "C:/Program Files/terraform" INFO Installing Terraform at "C:/Program Files/terraform" FATAL Could not create target binary: C:\Program Files\terraform.exe exit status 1

I guess it's a better solution. Should I use it ?

[MatthewJohn]

I've made an attempt with the informations from (https://stackoverflow.com/a/76285404/5093149) on branch PierreTechoueyres:fix-userBinPath-on-windows-1. With it and an unwritable path in my ~/.tfswitch.toml I obtain :

INFO Reading configuration from "C:\Users\MyName.tfswitch.toml" ERROR Could not create file "tmpfile" on "C:\Program Files" INFO Installing Terraform at "C:\Users\MyName\bin" INFO Switched Terraform to version "1.8.4"

Whereas with the actual PR it shows

INFO Reading configuration from "C:\Users\MyName.tfswitch.toml" INFO Found binary path "C:/Program Files/terraform" INFO Installing Terraform at "C:/Program Files/terraform" FATAL Could not create target binary: C:\Program Files\terraform.exe exit status 1

I guess it's a better solution. Should I use it ?

@PierreTechoueyres I do prefer the idea of still checking permissions. I seems weird that the permissions can't be checked without writing a file. Do you know what was going wrong (I assume https://github.com/warrensbox/terraform-switcher/compare/master...PierreTechoueyres:terraform-switcher:fix-userBinPath-on-windows-1#diff-a0b72d66e4d7d196755924401e35084604ef682057492feed9cb18b8199e2c5fL22 wasn't working correctly?)

The original code looks very similar to https://stackoverflow.com/a/49148866 - is there any debugging that can be done to check why it's not working as expected?

[PierreTechoueyres]

I think we don't understand each other. The branch fix-userBinPath-on-windows-1 has an write access check before using the custom path. Hence the output:

INFO Reading configuration from "C:\Users\MyName.tfswitch.toml" ERROR Could not create file "tmpfile" on "C:\Program Files" INFO Installing Terraform at "C:\Users\MyName\bin" INFO Switched Terraform to version "1.8.4"

Whereas the branch fix-userBinPath-on-windows hasn't this check and fail if the directory isn't writable:

INFO Reading configuration from "C:\Users\MyName.tfswitch.toml" INFO Found binary path "C:/Program Files/terraform" INFO Installing Terraform at "C:/Program Files/terraform" FATAL Could not create target binary: C:\Program Files\terraform.exe exit status 1

Both are compromises. fix-userBinPath-on-windows-1 does the check with an attempt to write in the directory, fix-userBinPath-on-windows doesn't this check and fail.

For your comment

I seems weird that the permissions can't be checked without writing a file. I agree with you but checking the permission on a directory is really tricky especially on Windows with the ACLs. This sole point could be the subject for an whole lib at all.

For now I think I prefer the fix-userBinPath-on-windows-1 branch.

The original code looks very similar to https://stackoverflow.com/a/49148866 - is there any debugging that can be done to check why it's not working as expected? The following points pop immediately on my mind but I'm certain there are others:

• the ACLs,
• the fact that's owner access that's checked and not the user access.

[MatthewJohn]

I think we don't understand each other.

I assume this is referring to my:

@PierreTechoueyres I do prefer the idea of still checking permissions. I seems weird that the permissions can't be checked without writing a file.

?

If so, I think we do (or at least, I'm think understanding what you're saying).. what I mean is: I'm not too keen on this PR, removing the check entirely for windows. I agree with you that I prefer the other (writing the temp dir to check for access). My comment was just that it seems strange that there isn't a way to reliable check write access without actually writing a file.

I do wonder if the following could be done instead (obviously relatively pseudocode):

func ChangeProductSymlink(product Product, binVersionPath string, userBinPath string) error {
  var possibleInstallLocations := {userBinPath, filePath.join(binDir, "bin")}
  var err error
  for pathItx := range possibleInstallLocations {
    err = CreateSymlink(binVersionPath, pathItx)
    if err == nil {
      break
    }
  }
  return err
}

So the "test" of whether we can write to the directory is the act of adding the symlink itself, rather than trying to evaluate the paths beforehand. Then we don't have to write test files on every execution.

and then update install to use the ChangeProductSymlink method, rather than re-inventing the wheel.

Just an idea :)

[yermulnik]

what I mean is: I'm not too keen on this PR, removing the check entirely for windows.

+1

[PierreTechoueyres]

@MatthewJohn: New version with your idea. Work on Linux, will check tomorrow on Windows.

[PierreTechoueyres]

This work as expected on Windows too.

[yermulnik]

@PierreTechoueyres Also please merge latest from master:

[PierreTechoueyres]

Is there something blocking the merge ?

[warrensbox]

@PierreTechoueyres Can you help us test your changes and let us know if you see any error?

Please test v1.2.0-alpha2 on your windows machine.

https://github.com/warrensbox/terraform-switcher/releases/tag/v1.2.0-alpha2

[PierreTechoueyres]

@warrensbox

@PierreTechoueyres Can you help us test your changes and let us know if you see any error?

Please test v1.2.0-alpha2 on your windows machine.

https://github.com/warrensbox/terraform-switcher/releases/tag/v1.2.0-alpha2

Sorry, didn't have time to check until now. v1.2.1 seem to work as expected.