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

Add Delay option #99

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from
Open

Add Delay option #99

wants to merge 2 commits into from

Conversation

daimhin
Copy link
Contributor

@daimhin daimhin commented Jun 25, 2019

Added Delay option to give number of seconds before disabling a server.
Changed Graceful option to use the Delay value if present on the command.

Description

Added -Delay option to the command that will give the number of seconds before disabling a server. Also changed the -Graceful to use the value supplied by a Delay.

Related Issue

#98

Motivation and Context

Adding a missing option that is part of the Nitro API.

How Has This Been Tested?

I ran the updated version of this cmdlet against several of our NetScalers to disable servers.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Added Delay option to give number of seconds before disabling a server.

Changed Graceful option to use the Delay value if present on the command.
Copy link
Collaborator

@iainbrighton iainbrighton left a comment

Choose a reason for hiding this comment

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

Hi @daimhin - thanks for the PR. Looking at the API and your code, I think we should add a parameter set to make sure only -Graceful 60 or -Delay 30 can be used, i.e. they cannot be specified together?

@@ -64,6 +67,8 @@ function Disable-NSLBServer {
[parameter(Mandatory,ValueFromPipeline = $true, ValueFromPipelineByPropertyName)]
[string[]]$Name = (Read-Host -Prompt 'LB server name'),

[int]$Delay,
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like the -Graceful and -Delay parameters should be mutually exclusive, e.g. use one or the other? If this is the case, we should introduce the Delay in a separate parameter set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have updated the code to use parameter sets that makes the Graceful and Delay options mutually exclusive.

@@ -87,7 +92,17 @@ function Disable-NSLBServer {
if ($Graceful -gt 0) {
$params.Add('delay', $Graceful)
} else {
$params.Add('delay', 0)
if ($PSBoundParameters.ContainsKey('Delay')) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

You don't need this check as [Int] parameters are always 0 unless otherwise specified. You should just be able to safely use $params.Add('delay', $Delay)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I simplified this section now that the Graceful and Delay are mutually exclusive.

- Add Parameter Sets to make the Graceful and Delay options mutually exclusive.  Also cleaned up the code where it checks for the values on these parameters.
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

3 participants