Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use go-homedir.Dir() everywhere #314

Merged
merged 4 commits into from
Mar 27, 2024

Conversation

kim0
Copy link
Contributor

@kim0 kim0 commented Aug 25, 2023

This can also close #117 since the binary will now respect $HOME which can be set to any location instead of ~/.terraform.versions

Signed-off-by: Ahmed Kamal <[email protected]>
@yermulnik
Copy link
Collaborator

Hey @kim0
Thank you for the contribution. I have two questions:

  1. I'm not quite really familiar with Golang internals and hence why is so huge removal change set for go.sum file?
  2. Looking at description and code of os.UserHomeDir(), I can see it relies upon HOME env var exclusively and returns error if it is not defined, which is quite rare though still viable condition (when HOME env var (or non-Linux OS alternative) is not defined), whilst go-homedir does its best to dig to the bottom of it and (despite running external commands) tries to determine user home dir much better than os.UserHomeDir() — what's the rationale behind this PR given the comparison on how these two determine user home dir? Thanks (and sorry if the question is dumb — I'm not a Golang folk 🤷🏻)

ps: this PR may also become stale as both owner (@warrensbox) and another contributor (@jukie) are more of not with this project rather than with it 😢

Signed-off-by: Ahmed Kamal <[email protected]>
@kim0
Copy link
Contributor Author

kim0 commented Aug 26, 2023

Hello @yermulnik

Thanks a lot for your feedback. I'm not that great with Golang either, I didn't think os.UserHomeDir() only looks at the HOME env var, but looking at its code, that does seem to be the case! A bit surprising since that is supposed to be the recommended way to work. Anyway, for now, I've switched everything to use go-homedir which does seem to do a better job!

It's sorta sad that the maintainers are no longer active mostly. My use-case involves running tfswitch with a read-only home directory on AWS lambda, which causes errors since tfswitch insists on writing to the home directory and does not respect $HOME. I hope I don't end up forking this repo and building releases! Really hope the maintainers can get in touch soon. Thanks!

@kim0 kim0 changed the title Use os.UserHomeDir() everywhere Use go-homedir.Dir() everywhere Aug 26, 2023
main.go Show resolved Hide resolved
@yermulnik
Copy link
Collaborator

My use-case involves running tfswitch with a read-only home directory on AWS lambda, which causes errors since tfswitch insists on writing to the home directory and does not respect $HOME.

Would the https://github.com/warrensbox/terraform-switcher#use-tfswitchtoml-file--for-non-admin---users-with-limited-privilege-on-their-computers be an option for your use case? You also can provide binary path as command line arg.

I hope I don't end up forking this repo and building releases! Really hope the maintainers can get in touch soon. Thanks!

🤞🏻

@kim0
Copy link
Contributor Author

kim0 commented Aug 29, 2023

be an option for your use case?

Thanks a lot for the reply @yermulnik .. wrt the suggested tfswitch -b or conf file, unfortunately this does not help. This is bec tfswitch is very stubborn wanting to write to the home directory. This was the reason for this PR. I confirmed -b doesn't help, by running in a container with the home directory /root mounted as read-only. You can see it fail below

root@5bc5fcb6879a:~# pwd
/root
root@5bc5fcb6879a:~# touch foo
touch: cannot touch 'foo': Read-only file system
root@5bc5fcb6879a:~# tfswitch -u -b /tmp/terraform
Creating directory for terraform binary at: /root/.terraform.versions
Unable to create directory for terraform binary at: /root/.terraform.versionspanic: mkdir /root/.terraform.versions: read-only file system

goroutine 1 [running]:
github.com/warrensbox/terraform-switcher/lib.CreateDirIfNotExist({0xc000327300, 0x19})
        /home/circleci/go/src/github.com/warrensbox/terraform-switcher/lib/files.go:112 +0x147
github.com/warrensbox/terraform-switcher/lib.GetInstallLocation()
        /home/circleci/go/src/github.com/warrensbox/terraform-switcher/lib/install.go:72 +0xca
github.com/warrensbox/terraform-switcher/lib.Install({0xc000300651, 0x5}, {0x7ffdda050f10?, 0xc0000920f0?}, {0x9e004d, 0x28})
        /home/circleci/go/src/github.com/warrensbox/terraform-switcher/lib/install.go:94 +0x70
main.installLatestVersion(0xc00016acb0, 0xc00016ad90)
        /home/circleci/go/src/github.com/warrensbox/terraform-switcher/main.go:237 +0x50
main.main()
        /home/circleci/go/src/github.com/warrensbox/terraform-switcher/main.go:179 +0xc9f
root@5bc5fcb6879a:~# echo $?
2
root@5bc5fcb6879a:~# ls -l /tmp/terraform
ls: cannot access '/tmp/terraform': No such file or directory

So given this failure, are you interested in merging this PR please? Let me know, thanks!

Signed-off-by: Ahmed Kamal <[email protected]>
@kim0
Copy link
Contributor Author

kim0 commented Aug 29, 2023

Sorry @yermulnik I confused you with being a maintainer .. Guess I'll have to wait more or find some other solution

@MatrixCrawler MatrixCrawler merged commit b7fc0d7 into warrensbox:master Mar 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

snap package cannot use "~/.terraform.version" and "/usr/local/bin"
3 participants