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

dsc resources description #106

Merged
merged 6 commits into from Jul 3, 2018
Merged

dsc resources description #106

merged 6 commits into from Jul 3, 2018

Conversation

avkaur
Copy link
Contributor

@avkaur avkaur commented Jul 2, 2018

dsc resources description updated for VM Attach scnearios


This change is Reviewable

@codecov-io
Copy link

codecov-io commented Jul 2, 2018

Codecov Report

Merging #106 into dev will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@         Coverage Diff         @@
##            dev   #106   +/-   ##
===================================
  Coverage    83%    83%           
===================================
  Files        19     19           
  Lines      2755   2755           
  Branches      4      4           
===================================
  Hits       2300   2300           
  Misses      451    451           
  Partials      4      4

Copy link
Member

@kwirkykat kwirkykat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a bullet point describing your changes under the Unreleased section in the module's README.md file.

@@ -19,6 +25,10 @@ $script:maxUserEnvVariableLength = 255
Retrieves the state of the environment variable. If both Machine and Process Target are
specified, only the machine value will be returned.

.DESCRIPTION
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resource description should not be added here since it was already added at the top

@@ -33,11 +43,12 @@ function Get-TargetResource
[OutputType([Hashtable])]
param
(
[Parameter(Mandatory = $true)]
[Parameter(Mandatory = $true, HelpMessage="Indicates the name of the environment variable for which you want to ensure a specific state.")]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most other resources only have HelpMessages added only to Set, but these are added in Get?

@@ -91,11 +95,12 @@ function Get-TargetResource
[CmdletBinding()]
param
(
[Parameter(Mandatory = $true)]
[Parameter(Mandatory = $true, HelpMessage="The name of the group to create, modify, or remove.")]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HelpMessage properties should be added to the parameters of SetTargetResource instead of GetTargetResource

Creates, modifies, or deletes a user.

#>

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are no parameter HelpMessages added for this resource?

[ValidateNotNullOrEmpty()]
[String[]]
$Name,

[Parameter(HelpMessage="Specifies whether the roles or features should be installed or uninstalled.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this work with the extra blank newline in a normal string?

@@ -85,26 +91,26 @@ function Get-TargetResource
[CmdletBinding()]
param
(
[Parameter(Mandatory = $true)]
[Parameter(Mandatory = $true, HelpMessage="The path to the archive file that should be expanded to or removed from the specified destination.")]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most other resources only have HelpMessages added only to Set, but these are added in Get?

@johlju johlju added the needs review The pull request needs a code review. label Jul 3, 2018
Copy link
Member

@kwirkykat kwirkykat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you decide to remove the help messages for the non-composite resources?

README.md Outdated
@@ -579,6 +579,7 @@ The following parameters will be the same for each process in the set:

### 2.8.0.0

* Added Description and Parameter description for composite resources
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move this release note into the Unreleased section above. Version 2.8.0.0 has already been released.

@johlju johlju added waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. and removed needs review The pull request needs a code review. labels Jul 3, 2018
@avkaur
Copy link
Contributor Author

avkaur commented Jul 3, 2018

verified the new line spacing for param description and our backend will remove the new line and display all in one line only

@kwirkykat kwirkykat merged commit 52d68a1 into PowerShell:dev Jul 3, 2018
@johlju johlju removed the waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. label Jul 3, 2018
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.

None yet

4 participants