-
Notifications
You must be signed in to change notification settings - Fork 1
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
Initial version #1
base: main
Are you sure you want to change the base?
Conversation
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.
It would be nice to have some tests for this module. It looks like you can use Vagrant on AppVeyor, we just have to ask AppVeyor to turn on nested virtualization. Let me know if you'd like me to do that.
If you can get Rundeck running in a container, that would work on AppVeyor.
I usually write an init.ps1 script that installs the software I'm testing and add that to my build script.
Demonstrates how to disable a job. | ||
#> | ||
param( | ||
[Parameter(Mandatory,ValueFromPipelineByPropertyName)] |
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.
nit: our standard for parameters is now:
# Parameter documentation.
[Parameter()]
[Type] $Name
e.g
# The job ID.
[Parameter(Mandatory, ValueFromPipelineByPropertyName)]
[Guid] $Id
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.
These should all be to the new standard now.
Demonstrates how to get a specific job usng its ID. | ||
#> | ||
param( | ||
[Parameter(Mandatory,ValueFromPipeline,ValueFromPipelineByPropertyName,ParameterSetName='ByName')] |
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.
nit: Separate Parameter
attribute properties with a comma and a space.
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.
These should all be to the new standard now. I also cleaned up some of the properties that had floated in from copy/paste.
} | ||
else | ||
{ | ||
$queryFilter = [ScriptBlock]::Create($Filter) |
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.
Maybe change the type of the $Filter
paramete to be [scriptblock]?
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.
I wanted to replicate the filter parameter from Get-ChildItem so passing in a string that filters like -Filter "test" to get all jobs with test in them. I wasn't able to get inline type transformation to work so this was my workaround.
else | ||
{ | ||
$queryFilter = [ScriptBlock]::Create($Filter) | ||
return ($allJobs | Where-Object { $_.name -like $queryFilter }) |
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.
If $queryFilter
is a script block, I believe you should be doing ($allJobs | Where-Object -FilterScript $queryFilter)
?
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.
This was a method I found I'll test with -FilterScript, but the intent here was to pass a string filter.
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.
I tested -FilterScript
and it didn't work as expected transforming the string value.
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.
Why are you converting $Filter
into a script block? Why aren't you return ($allJobs | Where-Object 'name' -Like $Filter)
?
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.
When I supply it as a string it wasn't doing pattern matching for me. I'll retest.
The `Get-RundeckJobExecution` function gets a job execution status from Rundeck. | ||
|
||
.EXAMPLE | ||
Get-RundeckJobExecution -ProjectName $refProject.Name -ID 'b090d4c8-585c-4330-8bc6-ad4783089dfd' |
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.
Example params don't match the function's signature.
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 be fixed in the next commit.
{ | ||
while ($jobRun.status -eq 'running') | ||
{ | ||
Start-Sleep -Seconds 10 |
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.
I would make this sleep duration a parameter.
[TimeSpan] $WaitInterval = [TimeSpan]'00:00:10'
# ...snip...
Start-Sleep -Milliseconds $WaitInterval.TotalMilliseconds
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.
Added as a function parameter.
RundeckAutomation/LICENSE
Outdated
@@ -0,0 +1,201 @@ | |||
Apache License |
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.
Usually this file goes in the repo root and we use a CopyFile
build task in whiskey.yml to copy it into the module root before packaging. That should probably be in the default whiskey.yml file.
Same with the NOTICE
and README.md
files: keep them in the root, and copy them into the module root before packaging.
Also, the NOTICE file is empty.
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.
These should be in the right locations now and populated.
RundeckAutomation/README.md
Outdated
@@ -0,0 +1,26 @@ | |||
# Overview |
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.
Remove this file.
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.
Removed.
@@ -116,8 +128,7 @@ | |||
Prerelease = '' | |||
|
|||
# ReleaseNotes of this module | |||
ReleaseNotes = @' | |||
'@ | |||
ReleaseNotes = 'https://github.com/webmd-health-services/RundeckAutomation/CHANGELOG.md' |
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.
Correct URL is https://github.com/webmd-health-services/RundeckAutomation/blob/main/CHANGELOG.md
.
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.
Corrected this.
@@ -14,7 +19,7 @@ Build: | |||
- Exec: | |||
OnlyBy: BuildServer | |||
Path: appveyor | |||
Argument: [ UpdateBuild, -Version, $(WHISKEY_SEMVER2) ] | |||
Argument: [ UpdateBuild, -Version, "$(WHISKEY_SEMVER2)+$(WHISKEY_BUILD_NUMBER)" ] | |||
|
|||
# Dot-sourcing files is expensive. Move all functions into .psm1 file to improve import speed. Do this before testing |
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.
Add this task after the existing MergeFile
task:
- CopyFile:
Path:
- LICENSE
- NOTICE
- README.md
- CHANGELOG.md
DestinationDirectory: RundeckAutomation
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.
Which means you'll want to add these lines to your .gitignore, too:
/RundeckAutomation/CHANGELOG.md
/RundeckAutomation/LICENSE
/RundeckAutomation/NOTICE
/RundeckAutomation/README.md
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.
Updated whiskey and the file locations.
Here's the content for the NOTICE file:
|
…ndeckAutomation into initial_version
I'm ready for a re-review. I think I've updated or replied to everything and the build looks green. |
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.
Can you take a look at my previous comments that have questions and respond to them?
param( | ||
# The job ID. | ||
[Parameter(Mandatory, ValueFromPipelineByPropertyName)] | ||
[GUID] |
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.
Nit a parameter's type and name should be on the same line and use the same case as the underlying type. e.g.
[Guid] $ID
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.
These should all be updated in the next commit.
Sorry this took so long to get back to, but I think I've responded to the issues and am ready for a review. |
I can't stress strongly enough that you should pass a mandatory session object to each function instead of having a global session object. I know many modules do it that way, but we should be following the pattern of PowerShell itself, not third-party modules. PowerShell has you create a session to something, then pass that session object around. Global variables are in general frowned upon and not recommended. Update [Parameter(Mandatory)]
[Object] $Session, |
else | ||
{ | ||
$queryFilter = [ScriptBlock]::Create($Filter) | ||
return ($allJobs | Where-Object { $_.name -like $queryFilter }) |
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.
Why are you converting $Filter
into a script block? Why aren't you return ($allJobs | Where-Object 'name' -Like $Filter)
?
$relativeUri = "project/$($Name)/export" | ||
$relativeUri = '{0}?{1}' -f $relativeUri, "exportConfigs=true&exportAll=false" | ||
$endpointUri = New-Object 'Uri' -ArgumentList @($_RundeckSession.Uri, $relativeUri) | ||
Invoke-WebRequest -ErrorAction 'Stop' -WebSession $_RundeckSession.WebSession -Method 'GET' -Uri $endpointUri -OutFile $Path |
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.
Update to use Invoke-RundeckRestMethod
instead of Invoke-WebRequest
. Keep all the logic for communicating with Rundeck in one function.
[String] $QueryString, | ||
|
||
# Content is XML format rather than JSON. | ||
[Switch] $ContentIsXML |
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.
Rename to BodyIsXml
.
} | ||
|
||
|
||
Start-Sleep -Seconds 1 |
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.
I would remove this Start-Sleep
in favor of the Wait
behavior below.
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.
I think I ran into a race condition where it didn't have a status on the job, but re-testing.
@@ -18,7 +18,7 @@ | |||
RootModule = 'RundeckAutomation.psm1' | |||
|
|||
# Version number of this module. | |||
ModuleVersion = '0.0.0' | |||
ModuleVersion = '0.1.1' |
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.
Set the version to 1.0.0
and update the whiskey.yml so you publish prerelease versions.
|
||
Describe 'Enable-RundeckJobSchedule' { | ||
|
||
BeforeAll { |
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.
Lint: usually BeforeAll
blocks are outside Describe
blocks.
init.ps1
Outdated
Set-StrictMode -Version 'Latest' | ||
$ErrorActionPreference = 'Stop' | ||
$InformationPreference = 'Continue' | ||
$VerbosePreference = 'Continue' | ||
|
||
Write-Information -InformationAction Continue -MessageData 'Starting init.ps1 script' |
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.
Instead of all the -InformationAction Continue
statements, add this to the top of the script:
$InformationPreference = 'Continue'
Find-Module -Name 'Prism' | Select-Object -First 1 | Install-Module -Force | ||
} | ||
prism install | Format-Table -Auto | ||
|
||
- Version: | ||
Path: RundeckAutomation\RundeckAutomation.psd1 | ||
Prerelease: |
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.
Standard is now:
- Version:
Path: RundeckAuomation\RundeckAutomation.psd1
Prerelease:
- main: ""
- "*": rc1
IncrementPrereleaseVersion: true
Initial cmdlets to set up a Rundeck session and do some basic job management.