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

Revert changing path.Join() to filepath.Join() (PR #3896) #4041

Merged
merged 1 commit into from
Nov 24, 2015
Merged

Revert changing path.Join() to filepath.Join() (PR #3896) #4041

merged 1 commit into from
Nov 24, 2015

Conversation

svanharmelen
Copy link
Contributor

PR #3896 added support for passing keys by content, but in this same PR
all references to path.Join() where changed to filepath.join().
There is however a significant difference between these two calls and
using the latter one now causes issues when running the Chef
provisioner on Windows (see issue #4039).

PR #3896 added support for passing keys by content, but in this same PR
all references to `path.Join()` where changed to `filepath.join()`.
There is however a significant difference between these two calls and
using the latter one now causes issues when running the Chef
provisioner on Windows (see issue #4039).
@svanharmelen
Copy link
Contributor Author

@phinze if you can have a look? Looked at adding some tests for this, but there is actually already a test in there for it. The problem is the test will only fail when executed on Windows, as the filepath.Join() call uses os.PathSeparator and so the test succeeds on Linux...

I'm afraid it didn't caught my eye when reviewing PR #3896, but I think this could/should be a reason to not wait too long with a 0.6.8 release 😞

But maybe wait a little to see if there are more obvious bug that need an immediate fix before doing so...

@phinze
Copy link
Contributor

phinze commented Nov 24, 2015

Ah dang, I definitely thought filepath.Join was "more correct" here for the same reason that it ended up breaking. I just had the logic backwards. 🙈

Will also look into getting a regular Windows test suite run into our processes.

phinze added a commit that referenced this pull request Nov 24, 2015
Revert changing `path.Join()` to `filepath.Join()` (PR #3896)
@phinze phinze merged commit 5e0b603 into hashicorp:master Nov 24, 2015
@svanharmelen svanharmelen deleted the b-chef-provisioner branch November 24, 2015 15:12
@joelhandwell
Copy link
Contributor

joelhandwell commented Aug 17, 2016

Will also look into getting a regular Windows test suite run into our processes.

@phinze @svanharmelen can appveyor be considered in addition to travis?

@svanharmelen
Copy link
Contributor Author

@joelhandwell I think that would be a good choice for making sure everything works on Windows as well.

Don't know how @phinze thinks about it and/or if there is already some Windows testing being done behind the scenes at HashiCorp?

@ghost
Copy link

ghost commented Apr 23, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Apr 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants