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

Replace the usage of go-homedir with os.UserHomeDir() #1521

Merged
merged 3 commits into from
Jan 8, 2025

Conversation

folliehiyuki
Copy link
Contributor

go-homedir project has been archived for a long time. os.UserHomeDir() returns the same value set so we can use it as a drop-in replacement. Sadly, some vendored modules still depend on go-homedir. The offended modules are github.com/awslabs/amazon-ecr-credential-helper and github.com/Azure/go-autorest, which are both pulled by the direct dependency github.com/google/go-containerregistry/pkg/authn/k8schain.

Ref: mitchellh/go-homedir#34

@pascalbreuninger
Copy link
Member

@folliehiyuki thanks for the contribution!
I understand that it's simpler to use os.UserHomeDir() instead of go-homedir but go-homedir has a couple of nice fallbacks like reading /etc/passwd if the user env var isn't specified. I don't exactly know if it will break if we change it but I'd also rather not take our chances here.

Since go-homedir is archived and small enough it might be just fine to put it's code somewhere under /pkg, wyt?

@folliehiyuki
Copy link
Contributor Author

folliehiyuki commented Jan 8, 2025

Sounds okay to me. I've copied the code from go-homedir with some modifications into pkg/util. If you prefer somewhere else I'll update it.

The only difference I see between go-homedir and os.UserHomeDir() is that the former always checks for $HOME even on Windows, so I added a test case for it. I don't use Windows so haven't actually verified the test.

@pascalbreuninger
Copy link
Member

Sounds good, thank you!

Copy link
Member

@pascalbreuninger pascalbreuninger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🙌

@pascalbreuninger
Copy link
Member

The linting CI failed and I've fixed it quickly. Hope you don't mind @folliehiyuki

@pascalbreuninger pascalbreuninger merged commit 3d19e41 into loft-sh:main Jan 8, 2025
18 of 24 checks passed
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.

2 participants