Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
storage: nix-only paths for block devices #7344
Conversation
| @@ -84,7 +82,7 @@ func (s *storageAttachmentInfoSuite) TestStorageAttachmentInfoPersistentDeviceNa | ||
| s.st.CheckCallNames(c, "StorageInstance", "StorageInstanceVolume", "VolumeAttachment", "BlockDevices") | ||
| c.Assert(info, jc.DeepEquals, &storage.StorageAttachmentInfo{ | ||
| Kind: storage.StorageKindBlock, | ||
| - Location: filepath.FromSlash("/dev/sda"), | ||
| + Location: "/dev/sda", |
jameinel
May 16, 2017
Owner
filepath is generally quite dangerous when you are talking about paths on another machine.
We ran into things like this around "bootstrap", etc trying to set /var/lib paths on the Ubuntu machine that is being started.
| } | ||
| if len(device.DeviceLinks) > 0 { | ||
| // return the first device link in the list | ||
| return device.DeviceLinks[0], nil | ||
| } | ||
| if device.DeviceName != "" { | ||
| - return filepath.Join(diskByDeviceName, device.DeviceName), nil | ||
| + return path.Join(diskByDeviceName, device.DeviceName), nil |
jameinel
May 16, 2017
Owner
Right. This is specifically one of those cases where you aren't talking about a local path, but a path on some other machine. We'll need to be careful about how we do them, but filepath is definitely not the answer.
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
jujubot
merged commit b91a449
into
juju:develop
May 16, 2017
1 check failed
github-check-merge-juju
Built PR, ran unit tests, and tested LXD deploy. Use !!.*!! to request another build. IE, !!build!!, !!retry!!
Details
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
axw commentedMay 16, 2017
Description of change
Storage is not supported on Windows currently,
and the block device paths will only ever work
on *nix machines. Don't use filepath.
QA steps
Run the tests on Linux and Windows.
Documentation changes
None.
Bug reference
http://reports.vapour.ws/releases/5227/job/run-unit-tests-win2012-amd64/attempt/3720