Skip to content
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

Updated Readme.md #113

Merged
merged 5 commits into from
Jul 3, 2017
Merged

Updated Readme.md #113

merged 5 commits into from
Jul 3, 2017

Conversation

bgelens
Copy link
Contributor

@bgelens bgelens commented Jul 2, 2017

  • Fixes: xHyper-V - Fix README.md markdown issues #112
  • Added missing or corrected DataType and Dsc attribute info
  • Added appveyor Dev badge
  • Added missing parameter info
  • Fixed / normalized examples
  • Added Opt In for Markdown common tests

This change is Reviewable

@bgelens bgelens added blocking release The issue or pull request is blocking the next release. Higher priority than label 'High priority'. needs review The pull request needs a code review. and removed blocking release The issue or pull request is blocking the next release. Higher priority than label 'High priority'. labels Jul 2, 2017
@bgelens
Copy link
Contributor Author

bgelens commented Jul 2, 2017

@johlju or @PlagueHO Can anyone of you sign this off? Thanks!

@johlju
Copy link
Member

johlju commented Jul 2, 2017

Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, 32 unresolved discussions.


README.md, line 4 at r1 (raw file):

The **xHyper-V** DSC module configures and manages a Hyper-V host using the
 **xVhd**, **xVMHyperV**, **xVMSwitch**, **xVhdFile**, **xVMDvdDrive**,

Do we need to list each resource here? Seems it could be missed when new resources are added. Could we just say something like "using Desired State Configuration." instead?
Or xSQLServer has the text 'The xSQLServer module contains DSC resources for deployment and configuration of SQL Server.'. So maybe something along those lines?

But if you want to keep it as-is, then that looks okay to me.


README.md, line 31 at r1 (raw file):

## Resources

* [**xVhd**](#xvhd) manages VHDs in a Hyper-V host.

Could you order these resources in alphabetical order?


README.md, line 31 at r1 (raw file):

## Resources

* [**xVhd**](#xvhd) manages VHDs in a Hyper-V host.

The resource and the resource folder is named xVHD (upper 'VHD'). This should reflect that. Or the resource should change the name to 'xVhd'.


README.md, line 47 at r1 (raw file):

 a Hyper-V virtual machine or to the management OS.

### xVhd

This resource has read parameters that should be listed here as well.

Example:
https://github.com/PowerShell/PSDscResources#read-only-properties-from-get-targetresource-4


README.md, line 47 at r1 (raw file):

 a Hyper-V virtual machine or to the management OS.

### xVhd

Between this and the parameters should we add the resource description? For now we could just copy the description from the ToC? If so, do that for all resources.


README.md, line 53 at r1 (raw file):

* **`[String]` ParentPath** (_Write_): Parent VHD file path, for differencing disk
* **`[Uint64]` MaximumSizeBytes** (_Write_): Maximum size of VHD to be created
* **`[String]` Generation** (_Write_): Virtual disk format: { Vhd | VHDx }

Add that the default value is 'Vhd'.


README.md, line 54 at r1 (raw file):

* **`[Uint64]` MaximumSizeBytes** (_Write_): Maximum size of VHD to be created
* **`[String]` Generation** (_Write_): Virtual disk format: { Vhd | VHDx }
* **`[String]` Ensure** (_Write_): Ensures that the VHD is Present or Absent

Add the value map to this as well. { *Present*\ | Absent }

Add that the text "Default value is 'Present'."
(also indicated with *Present* in the value map)


README.md, line 56 at r1 (raw file):

* **`[String]` Ensure** (_Write_): Ensures that the VHD is Present or Absent

### xVMHyperV

This resource has read parameters that should be listed here as well.


README.md, line 60 at r1 (raw file):

* **`[String]` Name** (_Key_): The desired VM name.
* **`[String]` VhdPath** (_Required_): The desired VHD associated with the VM.
* **`[String]` SwitchName** (_Write_): Virtual switch(es) associated with the VM.

Should be [string[]]


README.md, line 64 at r1 (raw file):

* **`[String]` State** (_Write_): State of the VM: { Running | Paused | Off }.
* **`[String]` Path** (_Write_): Folder where the VM data will be stored.
* **`[Uint32]` Generation** (_Write_): Virtual machine generation { 1 | 2 }.

Add that the default value is '1'.


README.md, line 66 at r1 (raw file):

* **`[Uint32]` Generation** (_Write_): Virtual machine generation { 1 | 2 }.
  Generation 2 virtual machines __only__ support VHDX files.
* **`[Boolean]` SecureBoot** (_Write_): Enables or disables secure boot

Add that the default value is $true.


README.md, line 81 at r1 (raw file):

* **`[Boolean]` RestartIfNeeded** (_Write_): If specified, will shutdown and
 restart the VM as needed for property changes.
* **`[String]` Ensure** (_Write_): Ensures that the VM is Present or Absent.

Add the value map to this as well. { *Present*\ | Absent }

Add that the default value is 'Present'.


README.md, line 83 at r1 (raw file):

* **`[String]` Ensure** (_Write_): Ensures that the VM is Present or Absent.
* **`[String]` Notes** (_Write_): Notes about the VM.
* **`[Boolean]` EnableGuestService** (_Write_): Enable Guest Service Interface

Add that the default value is $false.


README.md, line 86 at r1 (raw file):

 for the VM.

The following xVMHyper-V properties **cannot** be changed after VM creation:

Maybe move these up under the resource description? So this is shown before the parameters?


README.md, line 92 at r1 (raw file):

* Generation

### xVMSwitch

This resource has read parameters that should be listed here as well.


README.md, line 99 at r1 (raw file):

* **`[String[]]` NetAdapterName** (_Write_): Network adapter name(s)
 for external switch type
* **`[Boolean]` AllowManagementOS** (_Write_): Specify if the VM host

Add that the default value is $false.


README.md, line 101 at r1 (raw file):

* **`[Boolean]` AllowManagementOS** (_Write_): Specify if the VM host
 has access to the physical NIC
* **`[Boolean]` EnableEmbeddedTeaming** (_Write_): Should embedded NIC teaming

Add that the default value is $false.


README.md, line 103 at r1 (raw file):

* **`[Boolean]` EnableEmbeddedTeaming** (_Write_): Should embedded NIC teaming
 be used (Windows Server 2016 only)
* **`[String]` BandwidthReservationMode** (_Write_): Specify the QoS mode used

Add that the default value is 'NA'.

Also make NA italic in the value map.


README.md, line 106 at r1 (raw file):

 (options other than NA are only supported on Hyper-V 2012+):
 { Default | Weight | Absolute | None | NA }.
* **`[String]` Ensure** (_Write_): Ensures that the VM Switch is Present or Absent

Add the value map to this as well. { *Present*\ | Absent }

Add that the default value is 'Present'.


README.md, line 108 at r1 (raw file):

* **`[String]` Ensure** (_Write_): Ensures that the VM Switch is Present or Absent

### xVhdFile

Could you order these resources in alphabetical order?


README.md, line 108 at r1 (raw file):

* **`[String]` Ensure** (_Write_): Ensures that the VM Switch is Present or Absent

### xVhdFile

Why is it a class thatis named xFileDirectory in this schema?
https://github.com/bgelens/xHyper-V/blob/dev/DSCResources/MSFT_xVhdFileDirectory/MSFT_xVhdFileDirectory.schema.mof


README.md, line 108 at r1 (raw file):

* **`[String]` Ensure** (_Write_): Ensures that the VM Switch is Present or Absent

### xVhdFile

Missing parameter CheckSum.

Add default value is 'ModifiedDate' for parameter CheckSum.


README.md, line 111 at r1 (raw file):

[MSFT_xFileDirectory]

Should be [MSFT_xFileDirectory[]]


README.md, line 126 at r1 (raw file):

 file or physical hard disk volume for the added DVD drive.
* **`[String]` Ensure** (_Write_): Specifies if the DVD Drive should exist or not.
 { *Present* | Absent }. Defaults to Present.

Depending on how your write the others this might need to be revised.


README.md, line 128 at r1 (raw file):

 { *Present* | Absent }. Defaults to Present.

### xVMNetworkAdapter

This resource has read parameters that should be listed here as well.


README.md, line 139 at r1 (raw file):

* **`[String]` MacAddress** (_Write_): Use this to specify a Static MAC Address.
 If this parameter is not specified, dynamic MAC Address will be set.
* **`[String]` Ensure** (_Write_): Ensures that the VM Network Adapter is

Add the value map to this as well. { *Present*\ | Absent }

And add that the default value is 'Present'.


README.md, line 386 at r1 (raw file):

    xVMSwitch switch
    {
        Name = "Test-Switch"

Use single quotes on all strings with only text. Do this throughput these examples.


README.md, line 401 at r1 (raw file):

    }

    # Customize VHD by copying a folders/files to the VHD before a VM can be created

Comment block for this text.


README.md, line 412 at r1 (raw file):

create the generation 1 testVM out of the vhd.

Upper 'C' on Create.

Should be upper-case VHD?


README.md, line 1151 at r1 (raw file):

        [Parameter(Mandatory=$true, Position=0)]
        [ValidateScript({Test-Path -Path $_})]
        [string]

Maybe use [System.String]? If so, do this throughout.


README.md, line 1195 at r1 (raw file):

        [Parameter()]
        [ValidateSet ("Archive", "Hidden",  "ReadOnly", "System" )]
        $Attribute

No type on this one.


README.md, line 1229 at r1 (raw file):

    )

    Import-DscResource -Module xHyper-V

Are you sure it shouldn't be ModuleName?

https://msdn.microsoft.com/en-us/powershell/dsc/sxsresource


Comments from Reviewable

…ngelog. Linked to examples instead of duplication (created missing samples from Readme code)
@bgelens
Copy link
Contributor Author

bgelens commented Jul 3, 2017

@johlju Based on your comments I decided to do the entire overhaul based on how DscResources Readme is done.

Also added a change log and removed all example code from the Readme and replaced with links.

The example files are the same as the example code was in the Readme. A couple example files where missing, these are created from the code which was in the Readme.

Examples will be checked and if needed improved (new issue #114 )


Review status: 1 of 7 files reviewed at latest revision, 32 unresolved discussions.


README.md, line 4 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Do we need to list each resource here? Seems it could be missed when new resources are added. Could we just say something like "using Desired State Configuration." instead?
Or xSQLServer has the text 'The xSQLServer module contains DSC resources for deployment and configuration of SQL Server.'. So maybe something along those lines?

But if you want to keep it as-is, then that looks okay to me.

OK, I thought so myself when adding 2 or 3 which where missing. Changed it


README.md, line 31 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could you order these resources in alphabetical order?

Done.


README.md, line 31 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

The resource and the resource folder is named xVHD (upper 'VHD'). This should reflect that. Or the resource should change the name to 'xVhd'.

Done.


README.md, line 47 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

This resource has read parameters that should be listed here as well.

Example:
https://github.com/PowerShell/PSDscResources#read-only-properties-from-get-targetresource-4

Done.


README.md, line 47 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Between this and the parameters should we add the resource description? For now we could just copy the description from the ToC? If so, do that for all resources.

Done.


README.md, line 53 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add that the default value is 'Vhd'.

Done.


README.md, line 54 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add the value map to this as well. { *Present*\ | Absent }

Add that the text "Default value is 'Present'."
(also indicated with *Present* in the value map)

Done.


README.md, line 56 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

This resource has read parameters that should be listed here as well.

Done.


README.md, line 60 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Should be [string[]]

Done.


README.md, line 64 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add that the default value is '1'.

Done.


README.md, line 66 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add that the default value is $true.

Done.


README.md, line 81 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add the value map to this as well. { *Present*\ | Absent }

Add that the default value is 'Present'.

Done.


README.md, line 83 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add that the default value is $false.

Done.


README.md, line 86 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Maybe move these up under the resource description? So this is shown before the parameters?

Done.


README.md, line 92 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

This resource has read parameters that should be listed here as well.

Done.


README.md, line 99 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add that the default value is $false.

Done.


README.md, line 101 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add that the default value is $false.

Done.


README.md, line 103 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add that the default value is 'NA'.

Also make NA italic in the value map.

Done.


README.md, line 106 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add the value map to this as well. { *Present*\ | Absent }

Add that the default value is 'Present'.

Done.


README.md, line 108 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could you order these resources in alphabetical order?

Done.


README.md, line 108 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Why is it a class thatis named xFileDirectory in this schema?
https://github.com/bgelens/xHyper-V/blob/dev/DSCResources/MSFT_xVhdFileDirectory/MSFT_xVhdFileDirectory.schema.mof

I think this is done for Intellisense purposes and modeled after the File resource https://msdn.microsoft.com/en-us/powershell/dsc/fileresource

I'll add a definition of the class to the readme so people will know the properties. I'll copy in descriptions coming from the above link where applicable. (I won't say what values are default or not as from looking at the code this doesn't looks like the implementation reflects the documentation from the link above.

I won't judge the validity of this approach. I'll leave that to redesign work for the HQRM module.


README.md, line 108 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Missing parameter CheckSum.

Add default value is 'ModifiedDate' for parameter CheckSum.

Done.


README.md, line 111 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

[MSFT_xFileDirectory]

Should be [MSFT_xFileDirectory[]]

Done.


README.md, line 126 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Depending on how your write the others this might need to be revised.

Done.


README.md, line 128 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

This resource has read parameters that should be listed here as well.

Done.


README.md, line 139 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Add the value map to this as well. { *Present*\ | Absent }

And add that the default value is 'Present'.

Done.


README.md, line 386 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Use single quotes on all strings with only text. Do this throughput these examples.

OK. Instead of fixing the example code, I decided to remove it from the readme and link to the examples in the examples folder instead and untouched. Examples where duplicates with a few exceptions. These exceptions are now example files.


README.md, line 401 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Comment block for this text.

OK. Instead of fixing the example code, I decided to remove it from the readme and link to the examples in the examples folder instead and untouched. Examples where duplicates with a few exceptions. These exceptions are now example files.


README.md, line 412 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

create the generation 1 testVM out of the vhd.

Upper 'C' on Create.

Should be upper-case VHD?

OK. Instead of fixing the example code, I decided to remove it from the readme and link to the examples in the examples folder instead and untouched. Examples where duplicates with a few exceptions. These exceptions are now example files.


README.md, line 1151 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Maybe use [System.String]? If so, do this throughout.

OK. Instead of fixing the example code, I decided to remove it from the readme and link to the examples in the examples folder instead and untouched. Examples where duplicates with a few exceptions. These exceptions are now example files.


README.md, line 1195 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

No type on this one.

OK. Instead of fixing the example code, I decided to remove it from the readme and link to the examples in the examples folder instead and untouched. Examples where duplicates with a few exceptions. These exceptions are now example files.


README.md, line 1229 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Are you sure it shouldn't be ModuleName?

https://msdn.microsoft.com/en-us/powershell/dsc/sxsresource

OK. Instead of fixing the example code, I decided to remove it from the readme and link to the examples in the examples folder instead and untouched. Examples where duplicates with a few exceptions. These exceptions are now example files.


Comments from Reviewable

@johlju
Copy link
Member

johlju commented Jul 3, 2017

:lgtm:


Reviewed 6 of 6 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@bgelens bgelens merged commit f997b47 into dsccommunity:dev Jul 3, 2017
@joeyaiello joeyaiello removed the needs review The pull request needs a code review. label Jul 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants