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

Expand paths with tilde #512

Merged
merged 1 commit into from
Aug 30, 2019
Merged

Expand paths with tilde #512

merged 1 commit into from
Aug 30, 2019

Conversation

variadico
Copy link
Contributor

@variadico variadico commented Aug 23, 2019

Currently, if you pass a path with a tilde to nats.UserCredentials it fails
because it doesn't expand the tilde to mean the user's home directory.

This change adds support to expand paths with a tilde, such as "~/.nkeys".

Resolves #509

@coveralls
Copy link

coveralls commented Aug 23, 2019

Coverage Status

Coverage decreased (-0.2%) to 92.187% when pulling 02f4e95 on add-tilde-support into 69eb740 on master.

Copy link
Member

@derekcollison derekcollison left a comment

Choose a reason for hiding this comment

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

Need a test, and do we need a dependency, is the logic that complex?

@variadico
Copy link
Contributor Author

variadico commented Aug 23, 2019

No, the logic isn't complex. However, the dep seems to handle a lot of edge cases.
https://github.com/mitchellh/go-homedir/blob/master/homedir.go

If we only want to solve the simple case, then we could probably just do this.

// Wally suggested just checking HOME.
if strings.HasPrefix(path, "~") {
    path = filepath.Join(os.Getenv("HOME"), path[1:])
}

Maybe at some point we'll get an issue saying that there is a bug in the expansion logic. Or maybe not.

Another option is that we could just copy the file over into the nats repo (with all the license stuff).

@wallyqs
Copy link
Member

wallyqs commented Aug 23, 2019

IMO checking whether $HOME or $home are being defined in env when ~ is the first char from the path would be good enough, then can return an error saying explicitly saying that $HOME has to be defined for it to work.

@ripienaar
Copy link
Contributor

In my experience this handles the bulk of cases, hashi one is undeniably better but this is good enough for me.

func homeDir() (string, error) {
	if runtime.GOOS == "windows" {
		drive := os.Getenv("HOMEDRIVE")
		home := os.Getenv("HOMEDIR")

		if home == "" || drive == "" {
			return "", fmt.Errorf("Cannot determine home dir, ensure HOMEDRIVE and HOMEDIR is set")
		}

		return filepath.Join(os.Getenv("HOMEDRIVE"), os.Getenv("HOMEDIR")), nil
	}

	home := os.Getenv("HOME")

	if home == "" {
		return "", fmt.Errorf("Cannot determine home dir, ensure HOME is set")
	}

	return home, nil
}

@derekcollison
Copy link
Member

I prefer to limit the dependencies unless we absolutely have to have them.

@variadico
Copy link
Contributor Author

All right. Implemented homeDir. Added tests.
80090af

Wasn't sure if I should add tests for windows as well... I probably should, right?

@variadico
Copy link
Contributor Author

variadico commented Aug 23, 2019

Oookay, so turns out this homedir function is already in the stdlib.
https://golang.org/pkg/os/#UserHomeDir

The other one in os/user is bad because it uses cgo. This one in os, basically does what we were doing, i.e. just checking env vars.

So, I deleted the original function we had. I also cross compiled from Linux to Windows to verify os.UserHomeDir worked. It correctly printed the home dir in command prompt, powershell, and the linux subsystem.

One weird thing is I'm not sure how Windows users expect to give us a path. I added some Windows tests too, hopefully @ColinSullivan1 can take a look at those to see if they make sense.

nats_test.go Outdated

// missing USERPROFILE env var
{path: "~/fizz", wantErr: true},
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey, @ColinSullivan1. Are these paths normally how Windows folks would give us a path?

Copy link
Member

@ColinSullivan1 ColinSullivan1 Aug 24, 2019

Choose a reason for hiding this comment

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

Not normally, unless running in some type of non-standard shell. paths would be \\foo\\bar, with backslashes escaped for the test. However, programmatically, / is accepted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool. Backtick strings allow you to write a literal without escaping.

`\foo\bar` == "\\foo\\bar"

So is this fine?

nats.go Show resolved Hide resolved
nats_test.go Outdated
func TestExpandTildePath(t *testing.T) {
t.Run("unix", func(t *testing.T) {
if runtime.GOOS == "windows" {
t.Skip()
Copy link
Member

Choose a reason for hiding this comment

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

Let's see if we can get one of these from @ColinSullivan1

Copy link
Member

Choose a reason for hiding this comment

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

Will do...

nats.go Outdated
return p, nil
}

home, err := os.UserHomeDir()
Copy link
Member

Choose a reason for hiding this comment

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

Golang doesn't check for %HOMEDRIVE%%HOMEPATH% which is also used. It'd be more complicated but great for users if env var expansion in general was supported.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay. I think we can do this with os.ExpandEnv.

@variadico
Copy link
Contributor Author

os.UserHomeDir is only for Go 1.12 and up. 😬 Had to bring our version back. CI sets up tests for Go 1.11.

Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

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

The comment that I have is with the use of t.Errorf() instead of t.Fatalf(). We usually use the later and the reason is that there is no reason to continue the test when some error occurs.

nats_test.go Outdated Show resolved Hide resolved
@variadico variadico force-pushed the add-tilde-support branch 2 times, most recently from 3671bb5 to 20e4898 Compare August 26, 2019 22:56
Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

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

You still have 2 places with Errorf() instead of Fatalf().

nats_test.go Outdated Show resolved Hide resolved
nats_test.go Outdated Show resolved Hide resolved
Currently, if you pass a path with a tilde to nats.UserCredentials it fails
because it doesn't expand the tilde to mean the user's home directory.

This change adds support to expand paths with a tilde, such as "~/.nkeys".
@variadico
Copy link
Contributor Author

Cool, good catch. Updated.

Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

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

LGTM

@variadico variadico merged commit 6ffcfbc into master Aug 30, 2019
@variadico variadico deleted the add-tilde-support branch August 30, 2019 01:27
@derekcollison
Copy link
Member

Congrats! First PR merge!

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.

nats.UserCredentials doesn't support tilde character
7 participants