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

Normalize path usage to use forward slashes to make things more platform agnostic #100

Merged
merged 6 commits into from
Jan 13, 2017
Merged

Normalize path usage to use forward slashes to make things more platform agnostic #100

merged 6 commits into from
Jan 13, 2017

Conversation

cstavro
Copy link

@cstavro cstavro commented Jan 13, 2017

For your consideration, this is a different approach that fixes #89 .

I've changed the relevant filepath calls to route through a helper method which calls filepath.ToSlash to convert them to forward slashes.

The path/filepath pkg will convert slashes back and forth dependent upon the underlying OS so converting them all to forward slashes for the purposes of manipulating them within the application should be okay and also passes the tests!

I think this is a better approach than trying to explicitly handle two different path types throughout the app.

There are a few test failures around concurrency with dynamodb and deleting tables that are still in use.

@cstavro cstavro changed the title Normalize epath usage to use forward slashes to make things more platform agnostic Normalize path usage to use forward slashes to make things more platform agnostic Jan 13, 2017
@cstavro
Copy link
Author

cstavro commented Jan 13, 2017

Here's the latest test run with all the changes in this PR thus far.

test_output.txt

@brikis98
Copy link
Member

test_output.txt

Almost all of these test failures have to do with concurrent modifications to DynamoDB. We generate a random name for the table and the odds of collision should be very low, and yet, the test output shows multiple tests using the exact same name. Here's the code used to generate a unique ID for the table:

func uniqueId() string {
	const BASE_62_CHARS = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz"
	const UNIQUE_ID_LENGTH = 6 // Should be good for 62^6 = 56+ billion combinations

	var out bytes.Buffer

	random := rand.New(rand.NewSource(time.Now().UnixNano()))
	for i := 0; i < UNIQUE_ID_LENGTH; i++ {
		out.WriteByte(BASE_62_CHARS[random.Intn(len(BASE_62_CHARS))])
	}

	return out.String()
}

Perhaps time.Now().UnixNano() doesn't work properly on Windows and returns the same seed for everyone?

@brikis98
Copy link
Member

BTW, this is an interesting approach! Is the idea here to replace all path separators with a "/"? I assume Windows can handle forward slashes as well as backslashes when loading things from the file system?

@cstavro
Copy link
Author

cstavro commented Jan 13, 2017

BTW, this is an interesting approach! Is the idea here to replace all path separators with a "/"? I assume Windows can handle forward slashes as well as backslashes when loading things from the file system?

Effectively, yes. I did some testing and have had no issues in windows with both slashes. Additionally, the path/filepath pkg in Go does the OS mangling for you already anyway. The path pkg on its own does not.

@brikis98
Copy link
Member

Effectively, yes. I did some testing and have had no issues in windows with both slashes. Additionally, the path/filepath pkg in Go does the OS mangling for you already anyway. The path pkg on its own does not.

Awesome. In that case, I think this approach makes sense. The only thing left to figure out is why the DynamoDB stuff is failing, including even a panic in some cases. Do those tests pass if you run just one of them? E.g. go test -v -run TestAcquireLockHappyPath.

Copy link
Member

@brikis98 brikis98 left a comment

Choose a reason for hiding this comment

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

Overall code looks good. Just one minor comment.

return filepath.ToSlash(filepath.Join(elem...))
}

func CleanPath(path string) string {
Copy link
Member

Choose a reason for hiding this comment

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

Could you add comments to these two methods explaining what they do and why the should be used (i.e. for cross-OS compatibility).

@cstavro
Copy link
Author

cstavro commented Jan 13, 2017

Awesome. In that case, I think this approach makes sense. The only thing left to figure out is why the DynamoDB stuff is failing, including even a panic in some cases. Do those tests pass if you run just one of them? E.g. go test -v -run TestAcquireLockHappyPath

They seem to, yes. So this definitely has something to do with the parallelization of the tests. I'll take a look at the uniqueid function.

@cstavro
Copy link
Author

cstavro commented Jan 13, 2017

Fixed!
I changed the way the seeding of rand works so it only initializes it once and things now seem to work swimmingly.
This fixes ALL the tests on Windows now.

test_output.txt

@brikis98
Copy link
Member

@cstavro You rock. This is awesome work! Thank you!

Merging now :)

@brikis98 brikis98 merged commit 6540c3f into gruntwork-io:master Jan 13, 2017
@brikis98
Copy link
Member

Just created a new release here: https://github.com/gruntwork-io/terragrunt/releases/tag/v0.9.0. If the build passes, the new binaries should show up in a few minutes.

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.

Nested terragrunt file not working on Windows
2 participants