Normalize FileVarPath

Normalize FileVarPath

Fix bug 1271819 by normalizing cmd.FileVar
on set. Add a new test to ensure deploy
--config works with a config path starting
with tilde.

https://code.launchpad.net/~waigani/juju-core/config_rel_path_1271819/+merge/217942

(do not edit description out of merge proposal)

[waigani, 2014-05-01 16:31:44]

Please take a look.

[menn0, 2014-05-01 21:51:15]

Apart from possibly adding one more test, LGTM.

It's probably best to wait for another review from someone else before landing.

https://codereview.appspot.com/92950045/diff/1/cmd/filevar.go
File cmd/filevar.go (right):

https://codereview.appspot.com/92950045/diff/1/cmd/filevar.go#newcode21
cmd/filevar.go:21: func (f *FileVar) Set(v string) (err error) {
What about adding a test in filevar_test.go?

[axw, 2014-05-01 21:54:28]

On 2014/05/01 21:51:15, menno.smits wrote:
> https://codereview.appspot.com/92950045/diff/1/cmd/filevar.go#newcode21
> cmd/filevar.go:21: func (f *FileVar) Set(v string) (err error) {
> What about adding a test in filevar_test.go?

+1

[waigani, 2014-05-01 22:20:36]

Please take a look.

[axw, 2014-05-01 22:42:04]

LGTM

https://codereview.appspot.com/92950045/diff/20001/cmd/filevar_test.go
File cmd/filevar_test.go (right):

https://codereview.appspot.com/92950045/diff/20001/cmd/filevar_test.go#newcode42
cmd/filevar_test.go:42: c.Assert(config.String(), gc.Equals,
osenv.Home()+"/config.yaml")
filepath.Join(osenv.Home(), "config.yaml")

[waigani, 2014-05-01 22:48:00]

Please take a look.

[waigani, 2014-05-01 23:38:28]

Please take a look.

[gz, 2014-05-02 01:10:27]

A couple of suggested changes.

https://codereview.appspot.com/92950045/diff/60001/cmd/filevar.go
File cmd/filevar.go (right):

https://codereview.appspot.com/92950045/diff/60001/cmd/filevar.go#newcode22
cmd/filevar.go:22: f.Path, err = utils.NormalizePath(v)
Why add logic to set? The current path processing is done in Read, which seems
preferenable as it doesn't mess with the user input till it's actually needed.

https://codereview.appspot.com/92950045/diff/60001/cmd/filevar_test.go
File cmd/filevar_test.go (right):

https://codereview.appspot.com/92950045/diff/60001/cmd/filevar_test.go#newcode41
cmd/filevar_test.go:41: var config cmd.FileVar
This test should use fake home and actually read the filevar.

https://codereview.appspot.com/92950045/diff/60001/cmd/juju/deploy_test.go
File cmd/juju/deploy_test.go (right):

https://codereview.appspot.com/92950045/diff/60001/cmd/juju/deploy_test.go#ne...
cmd/juju/deploy_test.go:147: coretesting.Charms.BundlePath(s.SeriesPath,
"dummy")
You need fake home for this test.

[waigani, 2014-05-02 01:41:50]

Please take a look.

[waigani, 2014-05-02 22:29:13]

Please take a look.