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

Fix support for forward slashes with nested folders in cd_files for Windows #115

Merged
merged 1 commit into from Sep 12, 2022

Conversation

tempora-mutantur
Copy link
Contributor

@tempora-mutantur tempora-mutantur commented Jul 6, 2022

Packer recommends that we use "/" as the path separator. There is a
problem with using it on Windows with nested folders because of the following behaviour:

We might pass paths with forward slashes in StepCreateCD struct

The string rootFolder, err := tmp.Dir("packer_to_cdrom") makes rootFolder use system-based slashes, which is "\" for Windows.

This rootFolder is passed to s.AddFile(rootFolder, toAdd)

And a mix of two slashes ruins our replace logic while going through the visit function. We end up having different slashes for allDirs and discardPath in intermediaryDirs := strings.Replace(allDirs, discardPath, "", 1)

We change our replace login to don't depend on slashes.

The tests we had didn't cover such cases as we passed all files with paths constructed with filepath.Join(dir, fname) which returns paths with "\" on Windows. So we added a test case to specifically check forward slashes.

test

Thank you.

Packer recommends that we use "/" as the path separator. There is a
problem with using it on Windows because of the following behaviour:

We might pass paths with forward slashes in `StepCreateCD` struct

The string `rootFolder, err := tmp.Dir("packer_to_cdrom")` makes
rootFolder use system-based slashes, which is "\" for Windows.

This rootFolder is passed to `s.AddFile(rootFolder, toAdd)`

And a mix of two slashes ruins our replace logic while going through
the `visit` function. We end up having different slashes for `allDirs`
and `discardPath` in
`intermediaryDirs := strings.Replace(allDirs, discardPath, "", 1)`

We change our replace login to don't depend on slashes.

The tests we had didn't cover such cases as we passed all files with
paths constructed with filepath.Join(dir, fname) which returns paths
with "\\" on Windows. So we added a test case to specifically check
forward slashes.
@tempora-mutantur tempora-mutantur requested a review from a team as a code owner July 6, 2022 20:17
@tempora-mutantur tempora-mutantur changed the title Fix possibility to use forward slashes in cd_files for Windows Fix possibility to use forward slashes with nested folders in cd_files for Windows Jul 6, 2022
@tempora-mutantur
Copy link
Contributor Author

@nywilken Hi, very sorry for disturbing. Maybe you'll have a chance to review this

Copy link
Member

@nywilken nywilken left a comment

Choose a reason for hiding this comment

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

Hi @tempora-mutantur thanks the ping here. I've been meaning to come back to this change. At first I was a little confused on why the testing was not failing locally. But I did eventually see that I need to run it with the PACKER_ACC environment 🤦 .

This looks good to me and works as expected. I will get this in and released shortly.

@nywilken nywilken added the bug Something isn't working label Sep 12, 2022
@nywilken nywilken merged commit f0d8f56 into hashicorp:main Sep 12, 2022
@nywilken nywilken changed the title Fix possibility to use forward slashes with nested folders in cd_files for Windows Fix support for forward slashes with nested folders in cd_files for Windows Sep 12, 2022
@tempora-mutantur
Copy link
Contributor Author

@nywilken thank you very much

@nywilken
Copy link
Member

nywilken commented Sep 13, 2022

You got it! We work on getting these changes in tested and merged sooner for the future. Thanks for making the fix. Greatly appreciated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants