-
Notifications
You must be signed in to change notification settings - Fork 52
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
Refactor xOpticalDiskDrive to resolve edge case issues and support multi disk systems #140
Comments
hi @PlagueHO only comment I have was I thought 'Ensure' was a mandatory parameter, it is confusing in this instance however, I did think of removing the drive letter if Ensure was Absent. I'm happy with your approach, however. |
Thanks for getting back to me @kewalaka ! I really appreciate this! 😁 Ensure isn't mandatory and can sometimes make things more confusing. So I reckon in this situation we should remove it. But I'm not adverse to using it - I just felt after looking at the code it would actually simplify it a lot. |
No worries in which case I agree makes more sense to drop it. The multiple
drives thing is quite a rare case, at least in my xp, but it make sense
just to align with other disk resources. It's a shame the key field cant
accept a default but the example makes it clear. Who'd thought such a
little resource would be such a troublemaker :) thanks for the advice and
feedback it's been interesting watching this progress.
|
It definitely is the edge cases that tend to be the most pain - I totally understand on that one. I only picked it up because my development test server has multiple optical drives in it and when I executed the integration tests they failed. But you're right - it is a bit of a pity the DiskId can't be defaulted as that would make the MOF trivial. I still would have had to add IsSingleInstance parameter if I didn't do this (as @johlju pointed out) - so I didn't think it would be too much of an issue to add a DiskId. |
@PlagueHO I agree to this, would be much better. But Ensure still has a point since the resource is called xOpticalDiskDriveLetter. Changing to the key as you suggest would mean the following to say "I don't want a drive letter on optival drive 1".
Although, if your suggestion is also to rename the resource to xOpticalDiskDrive (as in the issue comment) I agree that Ensure parameter become strange. I can't remove the optical drive from the OS, which Another thing, I agree that your suggestion to use DiskId is good, we need to document that (in comment-based help) so it's clear why that name choice. Also, in the example it would be good to add (to the comment-based help) how the user finds the DiskId for the optical drive. |
Hi @johlju - I like the idea about renaming this resource to xOpticalDiskDrive. But I think we probably need to do some more thinking on the whole thing. I'll leave this open for a little bit before I do any more work - as somethings still feel off. |
I'm still knocking this change about and have actually think that the But the problem I have is whether or not to make
xOpticalDiskDriveLetter DismountOpticalDisk1
{
DiskId = 1
DriveLetter = 'X' # The actual value doesn't even matter
Ensure = 'Absent'
} The above just seems wrong. But perhaps it doesn't really matter too much because removal of drive letters from a specific optical disk is not a common scenario I'd expect to see.
xOpticalDiskDriveLetter DismountOpticalDisk1
{
DiskId = 1
Ensure = 'Present'
} This feels much worse as it'll result in a valid MOF that will have to throw exceptions at run time. So IHMO option 1 is the better option. Any thoughts from you guys @kewalaka , @timhaintz, @johlju on how you'd expect to use this? |
I like option 1. My use case is to change the drive letter to Z or get rid of it all together so I can use D: or E: for a data drive. Using Azure ARM templates, it seems like D: is the default for the optical drive.
Would work well. Thanks for all the thought and expertise @PlagueHO . I'm learning a lot, it is great. EDIT: How do you format your code nicely? |
Cool! Thanks @timhaintz - I think this makes the most sense to me too. |
@PlagueHO I vote for option 1 too. @timhaintz you format the code by using three backticks on a seperate row before and after the code. Or a single backtick before and after to make the ``` |
@johlju, @kewalaka , @timhaintz - Could I get some input from you guys: The more I look into this resource, the more I'm finding things that may cause confusion in the long run.
It has a limited scope and also fails in one case:
I think I could improve this by
So for a single optical disk drive system:
For a system with two optical disk drives:
I think we need to address these issues before the next DSC resource kit goes out. I'm happy to make the changes (I've already begun resolving the errors occurring in the edge cases).
So I need to know if anyone has any objections to this pattern? @johlju - I will probably close #139 without merging.
The text was updated successfully, but these errors were encountered: