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.

Sign up for GitHub

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

Jump to bottom

Fix relative path issues on Windows #96

Closed
wants to merge 5 commits into from
Closed

Fix relative path issues on Windows #96

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e04746b
Convert \ to / when getting relative path
Jan 10, 2017
a54fda6
Use platform specific root folders in relative path tests
Jan 10, 2017
4ebead5
Revert "Convert \ to / when getting relative path"
Jan 10, 2017
1f893e9
Fix for platform specific path separators in find_in_parent_folders a…
Jan 10, 2017
2114b9c
Change HCL path cleaning to prevent double escaping backslashes
Jan 11, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions config/config_helpers_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ func TestPathRelativeToInclude(t *testing.T) {
"child",
},
{
&IncludeConfig{Path: "/root/.terragrunt"},
&IncludeConfig{Path: rootFolder + "/.terragrunt"},
Copy link
Member

Choose a reason for hiding this comment

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

There are several other uses of /root in this file. Would you mind updating them the same way? That will help prevent these sorts of bugs in the future.

options.TerragruntOptions{TerragruntConfigPath: "/root/child/.terragrunt", NonInteractive: true},
"child",
},
Expand All @@ -36,7 +36,7 @@ func TestPathRelativeToInclude(t *testing.T) {
"child/sub-child/sub-sub-child",
},
{
&IncludeConfig{Path: "/root/.terragrunt"},
&IncludeConfig{Path: rootFolder + "/.terragrunt"},
options.TerragruntOptions{TerragruntConfigPath: "/root/child/sub-child/sub-sub-child/.terragrunt", NonInteractive: true},
"child/sub-child/sub-sub-child",
},
Expand Down
5 changes: 5 additions & 0 deletions config/test_constants_unix.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
// +build !windows

package config

var rootFolder = "/root"
5 changes: 5 additions & 0 deletions config/test_constants_windows.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
// +build windows

package config

var rootFolder = "C:/root"
2 changes: 2 additions & 0 deletions util/file.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"github.com/gruntwork-io/terragrunt/errors"
"io/ioutil"
"regexp"
"strings"
)

// Return true if the given file exists
Expand Down Expand Up @@ -83,6 +84,7 @@ func GetPathRelativeTo(path string, basePath string) (string, error) {
return "", errors.WithStackTrace(err)
}

relPath = strings.Replace(relPath, `\`,"/", -1)
Copy link
Member

@brikis98 brikis98 Jan 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Hm, I'm not sure this is the right place for the fix, nor quite the right fix. The bug, as explained here, is caused by the fact that Windows uses backslashes in its paths, but backslashes mean "escape" in HCL. So the fix is not to replace backslashes, but to escape them properly so they work in HCL.

Therefore, I think the proper fix is:

  1. Create a method called like cleanPathForHcl(path string) that escapes backslashes (i.e. turns them into a double backslash). Add a comment with the explanation above.
  2. Call that method at the end of the findInParentFolders and pathRelativeToInclude methods, since those methods are explicitly built to return valid HCL values.

Copy link
Author

Choose a reason for hiding this comment

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

So the problem this causes is that all the tests are using hard coded strings with / as the path separator. So this is going to require either some sort of a per platform mapping for test values (seems excessive), or a conversion from \ to / for the purposes of the test anyway (seems less than ideal).

Copy link
Author

Choose a reason for hiding this comment

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

Additionally, pathRelativeToInclude can actually call findInParentFolders indirectly through ResolveTerragruntConfigString. So simple escaping is actually double escaping in many scenarios.

Copy link
Member

@brikis98 brikis98 Jan 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Should be possible to add a helper to test_constants_unix.go like this:

func cleanPath(path string) string {
  return strings.Replace(path, "\\", "/", -1)
}

And in test_constants_windows:

func cleanPath(path string) string {
  return strings.Replace(path, "/", "\\", -1)
}

(Perhaps a better name would be test_helpers_unix.go and test_helpers_windows.go.)

You can then wrap all the paths in the tests with a call to cleanPath and it should take care of the cross-platform issues...

return relPath, nil
}

Expand Down