fix nasty edge case where we can't find guest additions on windows if they are on a different drive #5761
Conversation
… they are on a different drive
|
looking into test failures, and seeing wether I can add a few more filepath tests to DownloadableURL |
|
Don't forget to include UNC paths in the testcases that are prefixed with a double-backslash. Some of us Windows people desire that too. |
|
I mean, or you could just give a code review to that PR i submitted like forever ago... |
|
Reviewed. If you wanna work with me on this please let me know. |
|
Hey, just realized what you're working on adding with this PR and maybe this will help with conversions, but in my GH-2377 branch that I submitted a PR for, I added an interface to downloader called "LocalDownloader" which exposes a .toPath() method for converting a busted windows-url to a local path. You can see the different cases that I handled at : https://github.com/arizvisa/packer/blob/GH-2377/common/download.go#L361, so if you're trying to verify that the url is actually something valid that can be used, then maybe that .toPath() method can be copied or exposed somehow(?) |
|
oh, nice. I'll take a look. |
SwampDragons
force-pushed
the
fix_5713
branch
from
94d4d57 to
1549732
Compare
|
@AndrewSav I've tested this branch on my windows vm and it appears to have cleared up your issue. Will you please sanity-check me and confirm that it isn't causing you trouble? I can create you a binary if you need it. |
|
@SwampDragons, It can take up to 48 hours. A binary would be appreciated as it would speed things up a bit. |
|
packer.zip |
… risk breaking things with this bugfix.
mwhooker
changed the title
|
@SwampDragons testing it now. a tar without extension in the zip gave me a pause but I figured that out. |
|
@SwampDragons it seems to be working, thank you for the fix. I just tested my test case from the original issue, I cannot vouch if the fix did not break something else ;) |
|
@AndrewSav Glad to hear it. |
builder/virtualbox/ovf/config.go
Outdated
| errs = packer.MultiErrorAppend(errs, | ||
| fmt.Errorf("source file needs to exist at time of config validation: %s", err)) | ||
| } | ||
| _, err := common.FileExistsLocally(c.SourcePath) |
There was a problem hiding this comment.
since the first parameter isn't used, I feel like this should be rewritten to something like
func ValidateFileExistence(string) bool
and just return true or false
So instead of saying "return a 2x2 result matrix and we'll decide what to do", we can just wrap all our business logic in a yes/no answer.
You could also have it return an error instead of a bool if you feel the error from stat is useful
There was a problem hiding this comment.
oh I like this. you're completely right; as used right now I really want a simple yes or no answer.
|
left a comment/advice. feel free to take it or not. lgtm |
…URL policies made by the PR hashicorp#5761 merge. common/config.go: Added the ability for DownloadableURL to promote UNC paths to the SMB uri. Modified DownloadableURL to include the "./" prefix when a relative path is passed to it. Fix-up the DownloadableURL argument if on windows and incorrectly prefixed with "/".
…g to disk on the windows platform. Also added tests for relative paths/uris.
common/config_test.go:
Replaced instances of os.Mkdir and os.Create with tests that use
the existing "common/test-fixtures" mechanism.
Removed the runtime.GOOS test for the "FileExistsLocally" test,
as the functionality should work regardless of the platform.
Added some more comprehensive tests for the relative uri/pathing.
Replaced the Windows Object Manager name test as the Object
Manager's naming scheme is different from a UNC path.
Modified the FilePaths tests to support the policy of windows absolute
paths being prefixed with the `/` introduced with PR hashicorp#5761.
|
Is this change included in the latest 1.3.2 release? I'm still encountering this issue and want to know if I should file a separate issue or not. |
|
Please do open a new issue! This was merged several releases ago, so if you're seeing something similar it's new/different/has been reintroduced. |
|
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
locked and limited conversation to collaborators
fixes #5713
fixes #5700
See the gloriously detailed comment here for context: #5713 (comment)
I've tested this on osx, as well as on windows both on the same and on a different drive. I've confirmed that this fixes the issue reported in 5713, and it doesn't seem to break anything else. Would love some sanity-check confirmation from other windows users that this has fixed things.