-
Notifications
You must be signed in to change notification settings - Fork 6
Initialize attached disk in windows ARM #42
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
Conversation
aledsage
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but I've not tried running it and not reviewed the powershell commands thoroughly.
Any windows experts/users care to comment as well?
What should we do for automated tests? Should we add windows variants of brooklyn-blockstore/catalog/catalog.tests.bom? And/or a variant of AbstractVolumeManagerLiveTest for windows (but such live tests are never run unfortunately)?
| public void createFilesystem(AttachedBlockDevice attachedDevice, FilesystemOptions filesystemOptions) { | ||
| JcloudsMachineLocation machine = attachedDevice.getMachine(); | ||
| if (!(machine instanceof SshMachineLocation)) { | ||
| throw new IllegalStateException("Cannot create filesystem for "+machine+" of type "+machine.getClass().getName()+"; expected "+SshMachineLocation.class.getSimpleName()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we still throw this in an else block (if it's not SshMacheLocation or WinRmMachineLocation)?
| public MountedBlockDevice mountFilesystem(AttachedBlockDevice attachedDevice, FilesystemOptions options) { | ||
| JcloudsMachineLocation machine = attachedDevice.getMachine(); | ||
| if (!(machine instanceof SshMachineLocation)) { | ||
| throw new IllegalStateException("Cannot mount filesystem for "+machine+" of type "+machine.getClass().getName()+"; expected "+SshMachineLocation.class.getSimpleName()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, should we fail if it's not ssh or winrm?
f472d9a to
1ef1b2a
Compare
|
@aledsage I've made the changes you requested, and am happy to look at automated tests (I think the |
|
@nakomis can you try to grab someone to test it on windows, or say what test you ran (e.g. which cloud/image/configuration, and example yaml for doing that)? If it's not merged by Monday lunchtime then please do pester me again though! |
|
@aledsage Sure. Here's the test YAML I used: And here's the location definition I used (creds obfuscated): I'll see if I can find someone to test it... |
1ef1b2a to
3932f43
Compare
This supercedes #35