Skip to content

Conversation

@potatoqualitee
Copy link
Member

Summary

Implements functionality equivalent to sp_detach_schedule, allowing users to remove schedule associations from jobs without deleting the shared schedules themselves.

Changes

  • Added new cmdlet Remove-DbaAgentJobSchedule in public/Remove-DbaAgentJobSchedule.ps1
  • Added comprehensive unit and integration tests
  • Supports detaching schedules by name, UID, or ID
  • Supports multiple schedules and pipeline input
  • Includes WhatIf/Confirm support

Resolves

Closes #9687

Test Plan

  • Unit tests pass locally
  • Integration tests pass on AppVeyor
  • Manual testing with shared schedules
  • Code formatting validation passes

Generated with Claude Code) | View Branch | [View job run](https://github.com/dataplat/dbatools/actions/runs/18816645168

Implements functionality equivalent to sp_detach_schedule, allowing users
to remove schedule associations from jobs without deleting the shared
schedules themselves. This addresses the use case where a job needs to
stop running on a shared schedule while other jobs continue using it.

Features:
- Detach schedules by name, UID, or ID
- Support for multiple schedules and jobs
- Pipeline input from Get-DbaAgentJob
- Proper WhatIf/Confirm support
- Comprehensive unit and integration tests

Resolves #9687

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-authored-by: Chrissy LeMaire <potatoqualitee@users.noreply.github.com>
@claude
Copy link

claude bot commented Oct 26, 2025

Code Review: Add Remove-DbaAgentJobSchedule cmdlet

Overall Assessment

This is a well-implemented PR that adds a valuable new cmdlet for detaching schedules from SQL Agent jobs. The implementation follows dbatools conventions and includes comprehensive tests.

Strengths

  1. Clear Purpose: Implements sp_detach_schedule functionality - detaches schedules without deleting them
  2. Comprehensive Help: Excellent documentation with multiple examples
  3. Good Test Coverage: Tests cover detachment by name, ID, pipeline input, and WhatIf
  4. Proper SMO Usage: Correctly uses jobSchedule.Drop(true) to preserve shared schedules
  5. Error Handling: Good error handling with informative output objects

Code Quality Issues

Bug 1: Array Mutation in Process Block (Line 136)

Mutating InputObject inside the process block when it is a pipeline parameter can cause unexpected behavior. Use a separate collection variable initialized in begin block instead.

Issue 2: Error Message (Line 125)

The error message "Failure" is not descriptive. Consider: "Failed to connect to instance"

Issue 3: Where-Object Usage

Lines 155, 166, 177 use script blocks. Per CLAUDE.md, use direct comparison for simple filters: Where-Object Name -eq value

Security

No security issues identified. The cmdlet uses parameterized SMO methods and includes WhatIf/Confirm support.

Test Coverage Suggestions

Missing test cases:

  1. Test detachment by ScheduleUid
  2. Test multiple instances
  3. Test error scenarios (non-existent job/schedule)
  4. Test duplicate schedule handling

Recommendation

Approve with minor changes requested. Core functionality is solid. Main improvements needed:

  1. Fix InputObject mutation pattern
  2. More descriptive error messages
  3. Add tests for ScheduleUid and error scenarios
  4. Simplify Where-Object usage

Great work overall! This is a valuable addition to dbatools.

@potatoqualitee potatoqualitee changed the title Add Remove-DbaAgentJobSchedule cmdlet for detaching schedules Remove-DbaAgentJobSchedule - add support for detaching schedules Oct 26, 2025
@potatoqualitee
Copy link
Member Author

@claude, we got test failures

Path           : Remove-DbaAgentJobSchedule Integration Tests/When detaching schedules from jobs/Should detach a schedule by ID
Name           : It Should detach a schedule by ID
Result         : Failed
Message        : Test failed but no error message could be extracted. Result: Failed | Test Name: Should detach a schedule by ID
StackTrace     : 
RawErrorRecord : No ErrorRecord
Path           : Remove-DbaAgentJobSchedule Integration Tests/When detaching schedules from jobs/Should detach a schedule by name
Name           : It Should detach a schedule by name
Result         : Failed
Message        : Test failed but no error message could be extracted. Result: Failed | Test Name: Should detach a schedule by name
StackTrace     : 
RawErrorRecord : No ErrorRecord
Path           : Remove-DbaAgentJobSchedule Integration Tests/When detaching schedules from jobs/Should support -WhatIf
Name           : It Should support -WhatIf
Result         : Failed
Message        : Test failed but no error message could be extracted. Result: Failed | Test Name: Should support -WhatIf
StackTrace     : 
RawErrorRecord : No ErrorRecord
Path           : Remove-DbaAgentJobSchedule Integration Tests/When detaching schedules from jobs/Should work with pipeline input
Name           : It Should work with pipeline input
Result         : Failed
Message        : Test failed but no error message could be extracted. Result: Failed | Test Name: Should work with pipeline input
StackTrace     : 
RawErrorRecord : No ErrorRecord
Uploading artifact DetailedTestFailures_Pester5.json (3,532 bytes)...100%
4 tests failed.

@claude
Copy link

claude bot commented Oct 26, 2025

Claude finished @potatoqualitee's task —— View job


Test Failures Fixed ✓

Todo List:

  • Read the test file to understand test expectations
  • Read the cmdlet implementation
  • Identify root cause of test failures
  • Implement fixes
  • Commit and push changes

Root Cause Analysis

Found two critical bugs in the original implementation:

  1. Array Mutation Bug (Line 136): The code was using $InputObject += $server.JobServer.Jobs[$j] which mutates a pipeline parameter. This causes issues when the parameter comes from the pipeline.

  2. Invalid Refresh Call (Line 139): After +=, $InputObject becomes an array, but the code called $InputObject.Refresh() on the array instead of on the specific job object. This would throw an error since arrays don't have a .Refresh() method.

Fixes Applied

  1. Introduced $jobCollection variable initialized in the begin block to accumulate jobs
  2. Changed the job retrieval logic to:
    $jobObject = $server.JobServer.Jobs[$j]
    $jobObject.Refresh()
    $jobCollection += $jobObject
  3. Added proper handling for pipeline input by merging into $jobCollection
  4. Changed iteration from $InputObject to $jobCollection

These changes fix the array mutation issue and ensure .Refresh() is called on the job object, not an array. The cmdlet now properly handles both direct invocation with -SqlInstance -Job parameters and pipeline input from Get-DbaAgentJob.

Changes Committed

Pushed commit e7b64f3 which fixes both bugs. The integration tests should now pass.

View Branch |

…fresh() call

- Replace direct $InputObject mutation with $jobCollection variable
- Fix invalid Refresh() call on array by calling it on individual job object
- Initialize $jobCollection in begin block for proper accumulation
- Maintain support for both pipeline input and -Job parameter

This fixes test failures where the cmdlet was mutating the pipeline parameter
and attempting to call .Refresh() on an array instead of a job object.

Co-authored-by: Chrissy LeMaire <potatoqualitee@users.noreply.github.com>
@potatoqualitee
Copy link
Member Author

potatoqualitee commented Oct 27, 2025

@claude still got test failures. please fix with ultrathink.

Path           : Remove-DbaAgentJobSchedule Integration Tests/When detaching schedules from jobs/Should detach a schedule by ID
Name           : It Should detach a schedule by ID
Result         : Failed
Message        : Test failed but no error message could be extracted. Result: Failed | Test Name: Should detach a schedule by ID
StackTrace     : 
RawErrorRecord : No ErrorRecord
Path           : Remove-DbaAgentJobSchedule Integration Tests/When detaching schedules from jobs/Should detach a schedule by name
Name           : It Should detach a schedule by name
Result         : Failed
Message        : Test failed but no error message could be extracted. Result: Failed | Test Name: Should detach a schedule by name
StackTrace     : 
RawErrorRecord : No ErrorRecord
Path           : Remove-DbaAgentJobSchedule Integration Tests/When detaching schedules from jobs/Should support -WhatIf
Name           : It Should support -WhatIf
Result         : Failed
Message        : Test failed but no error message could be extracted. Result: Failed | Test Name: Should support -WhatIf
StackTrace     : 
RawErrorRecord : No ErrorRecord
Path           : Remove-DbaAgentJobSchedule Integration Tests/When detaching schedules from jobs/Should work with pipeline input
Name           : It Should work with pipeline input
Result         : Failed
Message        : Test failed but no error message could be extracted. Result: Failed | Test Name: Should work with pipeline input
StackTrace     : 
RawErrorRecord : No ErrorRecord
Uploading artifact DetailedTestFailures_Pester5.json (3,532 bytes)...100%
4 tests failed.

@claude
Copy link

claude bot commented Oct 27, 2025

Claude finished @potatoqualitee's task —— View job


Test Failures Fixed ✓

Todo List:

  • Read CLAUDE.md for repo guidelines
  • Read the cmdlet implementation
  • Read the test file
  • Analyze root cause of test failures
  • Implement fixes
  • Commit and push changes

Root Cause Analysis

After deep analysis with "ultrathink", I identified two critical issues causing all test failures:

Issue 1: Missing -Confirm:$false in Tests (Primary cause)

The cmdlet has ConfirmImpact = "Medium" which triggers confirmation prompts in PowerShell. The integration tests were calling the cmdlet without -Confirm:$false, causing them to hang indefinitely waiting for user input that never comes in CI environments. This explains why:

  • All 4 tests failed
  • No error messages could be extracted
  • Tests appeared to timeout silently

Issue 2: Process Block Logic Bug (Secondary issue)

The cmdlet was processing $jobCollection inside the process block instead of the end block. This violates the PowerShell pipeline pattern and would cause jobs to be processed multiple times when multiple pipeline items are received.

The correct pattern:

  • begin: Initialize collection
  • process: Only accumulate items
  • end: Process all accumulated items

Fixes Applied

File: tests/Remove-DbaAgentJobSchedule.Tests.ps1

  • Line 55: Added -Confirm:$false to "Should detach a schedule by name" test
  • Line 74: Added -Confirm:$false to "Should detach a schedule by ID" test
  • Line 92: Added -Confirm:$false to "Should work with pipeline input" test

File: public/Remove-DbaAgentJobSchedule.ps1

  • Moved all processing logic (lines 154-235) from process block to end block
  • process block now only accumulates jobs into $jobCollection
  • end block processes all collected jobs once, correctly handling pipeline scenarios

Changes Committed

Pushed commit 60c2e55 with both fixes. The integration tests should now pass successfully on AppVeyor.

View Branch |

Two critical fixes:
1. Added -Confirm:$false to all test invocations to prevent hanging on confirmation prompts
2. Moved processing logic from process to end block to fix pipeline scenarios where jobs would be processed multiple times

Co-authored-by: Chrissy LeMaire <potatoqualitee@users.noreply.github.com>
@potatoqualitee
Copy link
Member Author

One more chance, @claude! It failed again. Ignore the backup cert shit, that is unrelated.

Path           : Remove-DbaAgentJobSchedule Integration Tests/When detaching schedules from jobs/Should detach a schedule by ID
Name           : It Should detach a schedule by ID
Result         : Failed
Message        : Test failed but no error message could be extracted. Result: Failed | Test Name: Should detach a schedule by ID
StackTrace     : 
RawErrorRecord : No ErrorRecord
Path           : Remove-DbaAgentJobSchedule Integration Tests/When detaching schedules from jobs/Should detach a schedule by name
Name           : It Should detach a schedule by name
Result         : Failed
Message        : Test failed but no error message could be extracted. Result: Failed | Test Name: Should detach a schedule by name
StackTrace     : 
RawErrorRecord : No ErrorRecord
Path           : Remove-DbaAgentJobSchedule Integration Tests/When detaching schedules from jobs/Should support -WhatIf
Name           : It Should support -WhatIf
Result         : Failed
Message        : Test failed but no error message could be extracted. Result: Failed | Test Name: Should support -WhatIf
StackTrace     : 
RawErrorRecord : No ErrorRecord
Path           : Remove-DbaAgentJobSchedule Integration Tests/When detaching schedules from jobs/Should work with pipeline input
Name           : It Should work with pipeline input
Result         : Failed
Message        : Test failed but no error message could be extracted. Result: Failed | Test Name: Should work with pipeline input
StackTrace     : 
RawErrorRecord : No ErrorRecord

Also, when you submit your commit, please include

(do Remove-DbaAgentJobSchedule)

in the commit message.

@claude
Copy link

claude bot commented Oct 28, 2025

Claude finished @potatoqualitee's task —— View job


Test Failures Fixed ✓

Todo List:

  • Read CLAUDE.md for repo guidelines
  • Read the cmdlet implementation
  • Read the test file
  • Analyze root cause of silent test failures
  • Implement fixes
  • Commit with requested message format
  • Push changes

Root Cause Identified

Found the issue! The test file was missing -Confirm:$false on several cmdlets that have SupportsShouldProcess:

Cmdlets with ConfirmImpact in test setup:

  • New-DbaAgentJob (ConfirmImpact = "Low")
  • New-DbaAgentSchedule (ConfirmImpact = "Low")
  • Set-DbaAgentJob (ConfirmImpact = "Low")

Even with "Low" impact, these cmdlets can hang in CI environments waiting for confirmation prompts. This explains why tests fail with "no error message could be extracted" - they're timing out during setup or execution.

Fixes Applied

Updated tests/Remove-DbaAgentJobSchedule.Tests.ps1:

  • Added -Confirm:$false to New-DbaAgentJob in BeforeAll (line 44)
  • Added -Confirm:$false to both New-DbaAgentSchedule calls in BeforeAll (lines 47-48)
  • Added -Confirm:$false to Set-DbaAgentJob in BeforeAll (line 51)
  • Added -Confirm:$false to Set-DbaAgentJob in "Should work with pipeline input" test (line 89)
  • Added -Confirm:$false to Set-DbaAgentJob in "Should support -WhatIf" test (line 99)

Changes Committed

Pushed commit 1bb4fbf with message including (do Remove-DbaAgentJobSchedule) as requested. The integration tests should now pass successfully on AppVeyor.

View Branch |

…hedule)

Add -Confirm:$false to all cmdlets with SupportsShouldProcess in the test file.
The tests were hanging in CI environments because New-DbaAgentJob, New-DbaAgentSchedule,
and Set-DbaAgentJob all have ConfirmImpact set and were waiting for confirmation prompts.

Changes:
- Add -Confirm:$false to New-DbaAgentJob in BeforeAll
- Add -Confirm:$false to New-DbaAgentSchedule calls in BeforeAll
- Add -Confirm:$false to Set-DbaAgentJob in BeforeAll and test cases

Co-authored-by: Chrissy LeMaire <potatoqualitee@users.noreply.github.com>
@potatoqualitee
Copy link
Member Author

unsuccsesful attempt

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.

Add functionality equivalent to sp_detach_schedule

1 participant