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

(#63) Add -MachineContactTimeout parameter for New-CcmDeploymentStep #65

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

vexx32
Copy link
Member

@vexx32 vexx32 commented Jun 30, 2022

Description Of Changes

  • Small cleanup for clarity and removing a bit of clutter in the code
  • Added -MachineContactTimeout parameter to New-CcmDeploymentStep
  • Renamed -ExecutionTimeoutMinutes parameter to match the new one as -ExecutionTimeout, and added an alias to the old name to avoid breaking folks

Motivation and Context

Previously, ChocoCCM was unable to set the machine contact timeout for a deployment step. With this change, we add the new parameter and allow users to select a contact timeout according to their needs. Just like in the CCM UI, the value is defaulted to zero.

Testing

  1. Create a new CCM deployment, either in the CCM UI or via ChocoCCM
  2. New-CcmDeploymentStep -Deployment $deploymentName -Name "testing machine contact timeout" -Type Basic -ChocoCommand Install -PackageName 7zip -MachineContactTimeout 5
  3. In the CCM UI, verify the new step was created and has the machine contact timeout listed as 5 minutes.

Change Types Made

  • Bug fix (non-breaking change)
  • Feature / Enhancement (non-breaking change)
  • Breaking change (fix or feature that could cause existing functionality to change)
  • PowerShell code changes.

Related Issue

Fixes #63

Change Checklist

  • Requires a change to the documentation
  • Documentation has been updated
  • Tests to cover my changes, have been added
  • All new and existing tests passed.
  • PowerShell v2 compatibility checked.

@vexx32 vexx32 requested a review from steviecoaster June 30, 2022 19:05
vexx32 added 2 commits July 6, 2023 15:49
Ensure attribute usage has consistent casing, reformat the hashtable for
the request body to be easier to read and use consistent casing for its
keys and variables.

Rewrote small portions of script that were redundant, overly
complicated or difficult to read.
Previously, ChocoCCM was unable to set the machine contact timeout.

With this change, we add the new parameter and allow users to select a
contact timeout according to their needs.

Just like in the CCM UI, the value is defaulted to zero.
@vexx32 vexx32 force-pushed the 63-add-missing-param branch from cbbf577 to 9bea49f Compare July 6, 2023 19:58
@vexx32 vexx32 requested a review from corbob July 6, 2023 20:02
Copy link
Member

@corbob corbob left a comment

Choose a reason for hiding this comment

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

LGTM!

Comment on lines 165 to 168
executionTimeoutInSeconds = $ExecutionTimeout
machineContactTimeoutInMinutes = $MachineContactTimeout
requireSuccessOnAllComputers = $RequireSuccessOnAllComputers
failOnError = $FailOnError
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
executionTimeoutInSeconds = $ExecutionTimeout
machineContactTimeoutInMinutes = $MachineContactTimeout
requireSuccessOnAllComputers = $RequireSuccessOnAllComputers
failOnError = $FailOnError
executionTimeoutInSeconds = $ExecutionTimeout
machineContactTimeoutInMinutes = $MachineContactTimeout
requireSuccessOnAllComputers = $RequireSuccessOnAllComputers.IsPresent
failOnError = $FailOnError.IsPresent

JSON serialization doesn't handle these well, it treats a [switch] parameter object as an object with the form { IsPresent: true } rather than a raw boolean, make sure we take the inner property explicitly (this should be fixed below for consistency as well)

}
name = $Name
deploymentPlanId = (Get-CCMDeployment -Name $Deployment).id
deploymentStepGroups = @(
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
deploymentStepGroups = @(
deploymentStepGroups = [System.Collections.Generic.List[psobject]]@(

This cast ensures the collection is always preserved during json serialization; it seems some versions of powershell unwrap a one-element array, but leave other collection types intact.

Copy link
Member

@corbob corbob left a comment

Choose a reason for hiding this comment

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

Two things I noticed testing this on Windows PowerShell:

  1. TargetGroup is required, or you get an error (although the deployment step is still created...)
  2. When it is provided, the group is not actually targeted for the deployment.
2023-07-21_16-12-54

@vexx32 vexx32 force-pushed the 63-add-missing-param branch from 04369d5 to 5f26f8e Compare July 24, 2023 21:06
@vexx32
Copy link
Member Author

vexx32 commented Jul 24, 2023

@corbob @JPRuskin I've pushed up a fix for that, thanks for catching that, Cory!

I think this current version is pretty robust for both PS 5 and 7 in my testing. The json serialisation differnces are doing my head in to be honest 😂

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.

New-CcmDeploymentStep - missing MachineContactTimeoutInMinutes parameter
2 participants