Refactor xOpticalDiskDrive to resolve edge case issues and support multi disk systems

[PlagueHO]

@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:

1. Supports only a single optical disk
2. Fails if the drive is not assigned to a letter at all
3. Having an "Ensure" parameter adds complexity that is not required: it would be easier to allow setting DriveLetter to empty string to dismount the drive letter.

I think I could improve this by

1. adding a DiskId parameter that is the resource Key (removes requirement for IsSingleInstance key). This would be the number of the optical disk number you want to set the DriveLetter for. In a single optical disk system this would be just set to a 1 (I can't default because it is a key). Note: I called this DiskId to match the other resources in this module (so would allow us to use different disk identifier values in future without requiring a breaking change).
2. Remove the 'Ensure' parameter because it would be easier to just allow DriveLetter to be an empty string for removing the drive letter.

So for a single optical disk drive system:

        xOpticalDiskDriveLetter MountOpticalDiskToZ
        {
            DiskId = 1
            DriveLetter = 'Z'
        }

For a system with two optical disk drives:

        xOpticalDiskDriveLetter RemoveOpticalDisk1DriveLetter
        {
            DiskId = 1
        }
        xOpticalDiskDriveLetter AssignOpticalDisk2DriveLetterZ
        {
            DiskId = 2
            DriveLetter = 'Z'
        }

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.

[kewalaka]

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.

[PlagueHO]

Thanks for getting back to me @kewalaka ! I really appreciate this! :grin:

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.

[kewalaka]

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.

[PlagueHO]

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.

[johlju]

@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".

        xOpticalDiskDriveLetter DismountOpticalDisk1
        {
            DiskId = 1
            Ensure   = 'Absent'
        }

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 Ensure = 'Absent' would suggest with that resource module name.

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.

[PlagueHO]

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.

[PlagueHO]

I'm still knocking this change about and have actually think that the Ensure parameter might still be required. The reason is that en empty string on a required parameter does not seem to be supported (I've never needed to implement this anywhere).

But the problem I have is whether or not to make DriveLetter a required parameter or not.

1. If DriveLetter is required then this is what removing of a drive letter looks like:

       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.

2. If DriveLetter is not required then this config is possible (but illegal):

       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?

[timhaintz]

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. For me:

    xOpticalDiskDriveLetter DismountOpticalDisk1
    {
        DiskId = 1
        DriveLetter = 'D' # The actual value doesn't even matter
        Ensure   = 'Absent'
    }

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? EDIT 2: Got it, more new line spaces, thanks. :)

[PlagueHO]

Cool! Thanks @timhaintz - I think this makes the most sense to me too.

[johlju]

@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 code block inline.

``` code ```