OctopusDeploy / Library

| Public | A repository of step templates and other community-contributed extensions to Octopus Deploy
Other
171 stars 504 forks source link

New step template to update tentacles #1443

Closed REOScotte closed 1 year ago

REOScotte commented 1 year ago

Background

We need a way to upgrade tentacles as part of an infrastructure update project. This template uses the api to trigger a task to upgrade the tentacle on finely targeted machines.

Results

This step template can be included in a project to upgrade tentacles.

Before

After

Pre-requisites

REOScotte commented 1 year ago

Requested changes implemented

twerthi commented 1 year ago

I've a couple of requested changes and one in case you didn't know suggestion that isn't required.

In the PowerShell, lines 30, 48, and 58, please update the if statements to explicitly check for null ( if ($null -eq <variable)). I've had issues in the past where I was doing this type of check and it was passing unexpectedly. Lines 87 and 94 are fine as the variables are converted to boolean explicitly.

The suggestion I had was making use of the parameters for the Split calls, there is a way to do what you're doing natively by using StringSplitOptions.RemoveEmptyEntries (https://stackoverflow.com/questions/46467258/c-sharp-split-string-and-remove-empty-string). Your code is fine and doesn't need to be changed, but wasn't sure if you knew about that trick.

REOScotte commented 1 year ago

No, I was not aware of StringSplitOptions.RemoveEmptyEntries, so thanks for that pointer. That said, after more experimentation .Trim() removes the empty entries as well, so I simplified the code for that. I also add a few more $null checks and a verbose list of machines when WhatIf is not set.

REOScotte commented 1 year ago

Also, I'd be interested to see instances where if ($Var) {} passes when $Var is null. That can happen with collections of nulls - $Var = $null, $null if ($Var) {"oops"}

But, I would suggest that should pass since you do have a collection. I know there's some specific rules with powershells type coercion. I'm just interested in corner cases I've not run into.

twerthi commented 1 year ago

Also, I'd be interested to see instances where if ($Var) {} passes when $Var is null. That can happen with collections of nulls - $Var = $null, $null if ($Var) {"oops"}

But, I would suggest that should pass since you do have a collection. I know there's some specific rules with powershells type coercion. I'm just interested in corner cases I've not run into.

I'll have to see if I can find some in my older code, I've switched to using the other method a while ago. I'll see if I can get this reviewed today, thank you for making the changes.

REOScotte commented 1 year ago

You were right, of course. I see now where .Trim() isn't quite cutting it. I added back in [StringSplitOptions]::RemoveEmptyEntries. I think .Trim() would have been fine in this instance, but I'd rather be more thorough.