Invoke-DbaDbLogShipping - Add Azure blob storage support

[potatoqualitee]

Summary

Adds native Azure blob storage support for log shipping, making dbatools the first major SQL Server toolset to support log shipping directly to Azure blob storage. This enables cloud-native disaster recovery and simplifies Managed Instance migrations.

New Features

• AzureBaseUrl parameter for Azure blob storage container URL
• AzureCredential parameter for storage account key authentication
• Support for both SAS token and storage account key authentication
• Smart path handling for Azure URLs vs traditional UNC paths
• SQL Server 2012+ version validation for Azure backup support
• Optimized architecture - backups go directly to cloud, no copy job needed

Modified Functions

• Invoke-DbaDbLogShipping: Added Azure parameter set and validation
• New-DbaLogShippingPrimaryDatabase: Azure URL support for primary setup
• New-DbaLogShippingSecondaryPrimary: Azure URL support for secondary setup

Benefits

• No file share infrastructure required
• Cost-effective DR using Azure blob storage
• Geographic flexibility for primary/secondary locations
• Perfect for Azure SQL Managed Instance migrations
• Maintains full backward compatibility with traditional file share log shipping

Closes #5278

Generated with Claude Code

[potatoqualitee]

@claude Path : Invoke-DbaDbLogShipping/Parameter validation/Should have the expected parameters
Name : It Should have the expected parameters
Result : Failed
Message : ErrorRecord: Expected $null or empty, but got @(@{InputObject=AzureBaseUrl; SideIndicator==>}, @{InputObject=AzureCredential; SideIndicator==>}).
StackTrace :
RawErrorRecord : Expected $null or empty, but got @(@{InputObject=AzureBaseUrl; SideIndicator==>}, @{InputObject=AzureCredential; SideIndicator==>}).

[claude]

Claude Code is working…

I'll analyze this and get back to you.

View job run

[potatoqualitee]

@claude try again

[claude]

Claude finished @potatoqualitee's task —— View job

---

Test fix completed

I've updated the parameter validation test to include the new Azure parameters.

Tasks

• Locate the test file for Invoke-DbaDbLogShipping
• Add AzureBaseUrl and AzureCredential to expected parameters
• Commit and push changes
• Verify fix

Changes Made

Updated tests/Invoke-DbaDbLogShipping.Tests.ps1:93-94 to add the two new Azure parameters to the expected parameters array:

• AzureBaseUrl
• AzureCredential

The parameter validation test should now pass when the CI runs again.
|

[claude]

PR Review: Invoke-DbaDbLogShipping - Add Azure blob storage support

This is an excellent feature addition that brings native Azure blob storage support to log shipping! The implementation is solid and follows dbatools conventions well.

---

Strengths

Code Quality

• Clean parameter set design: The use of separate parameter sets (FileShare and AzureBlob) is the correct approach for mutually exclusive scenarios
• Proper validation: Azure URL format validation with regex patterns is well-implemented
• Version checking: SQL Server 2012+ requirement for Azure backups is correctly enforced
• Backward compatibility: Traditional file share log shipping remains fully functional
• Style compliance: Code follows dbatools style guide (double quotes, proper formatting)

Architecture

• Smart optimization: Recognizing that Azure backups don't need a separate copy job (backups go directly to cloud) is a key insight
• Flexible authentication: Supporting both storage account key and SAS token authentication provides good flexibility
• Path handling: Dynamic path construction for both Azure URLs and UNC paths works well

Documentation

• Comprehensive help: New parameters are well-documented with clear descriptions
• Good examples: Azure-specific examples demonstrate both authentication methods
• Proper authorship: Credits both original author and Claude for Azure support

---

Issues to Address

1. CRITICAL: Backticks Usage

The existing code (not introduced in this PR) uses backticks extensively for line continuation, which violates the dbatools style guide.

CLAUDE.md requirement: NEVER suggest or use backticks for line continuation. Backticks are an anti-pattern in modern PowerShell development.

Recommended fix: Convert to splats for 3+ parameters (this is a pre-existing issue, not introduced by this PR, but good opportunity to modernize).

2. Potential Bug: Azure Credential Validation

The code doesn't validate that the Azure credential exists before attempting to use it. If a user provides an AzureCredential name that doesn't exist on the SQL Server instance, the backup will fail with a potentially unclear error.

Recommendation: Add credential existence check before attempting backups and restores to provide clear error messages.

3. SAS Token Credential Naming Convention

When AzureCredential is not provided, the code assumes a credential exists named exactly as the AzureBaseUrl. The documentation and error messages don't clearly explain this convention to users.

Recommendation: Add explicit validation and clear error messages explaining the naming convention.

4. Minor: Regex Pattern Strictness

The Azure URL regex could be more permissive or the input could be normalized to lowercase to prevent case-sensitivity issues (though Azure storage accounts are lowercase-only, users might type it with caps).

---

Test Coverage

Current State: Parameter validation test updated correctly

Gaps: The PR only adds parameter validation tests. Consider adding integration tests for:

1. Azure URL validation - Test that invalid URLs are rejected
2. Version validation - Test that SQL Server 2008 R2 rejects Azure setup
3. Credential validation - Test behavior when credentials are missing

Per CLAUDE.md: When adding new parameters or functionality, include reasonable tests that verify the new feature works. Be focused and practical - test your changes, not everything.

---

Security Considerations

Positive:

• No credential exposure: Credentials are referenced by name, not passed as plaintext
• Proper authentication methods: Both SAS token and storage account key are valid Azure authentication patterns
• No SQL injection risks: Parameters are passed to cmdlets, not concatenated into SQL strings

Recommendations:

1. Document credential security requirements in the .DESCRIPTION section
2. Warn about credential scope: Make clear that credentials must exist on both source and destination instances

---

Recommendations Summary

Must Fix:

1. Validate Azure credentials exist before attempting backup/restore operations
2. Add clear error messages for SAS token credential naming convention
3. Document credential requirements more explicitly in help text

Should Fix (Style Compliance):
4. Convert backticks to splats (pre-existing issue, but good opportunity to modernize)
5. Add reasonable integration tests for Azure-specific validation

Nice to Have:
6. Normalize Azure URL input (ToLower) to prevent case-sensitivity issues
7. Add more verbose logging for Azure-specific paths and decisions

---

Overall Assessment

Rating: 8.5/10 - Excellent feature implementation with minor issues to address

This is a well-thought-out feature that genuinely adds value to dbatools. The code is clean, follows most conventions, and the architecture is sound. The main concerns are around credential validation and pre-existing style issues with backticks.

Recommendation: Approve with requested changes

Once the credential validation is added and error messages are clarified, this will be production-ready. The backtick cleanup can be done as a follow-up refactoring if preferred.

Great work on bringing modern Azure support to log shipping!

---

Review generated by Claude Code with dbatools style guide compliance checking

[potatoqualitee]

@claude please fix per your suggestions including moving to splats

[claude]

Claude finished @potatoqualitee's task —— View job

---

Implementation Complete

I've successfully implemented all the review suggestions for the Azure blob storage log shipping feature.

Changes Made

1. Backtick to Splat Conversion (Style Compliance)

   • Converted 10 instances of backtick line continuation to properly formatted splats
   • All hashtables now have perfectly aligned equals signs per dbatools style guide
   • Used descriptive splat names: $splatPrimary, $splatRestore, $splatBackupSchedule, etc.

2. Azure Credential Validation (Bug Fix)

   • Added credential existence checks in New-DbaLogShippingPrimaryDatabase.ps1:150-160
   • Added credential existence checks in New-DbaLogShippingSecondaryPrimary.ps1:162-186
   • Validation occurs before attempting backup/restore operations
   • Clear error messages guide users to create credentials using New-DbaCredential

3. Enhanced Error Messages (User Experience)

   • Explained SAS token credential naming convention clearly
   • Error messages now specify exactly which credential is missing
   • Instructions included for creating credentials with storage account key or SAS token

4. Unit Tests Added (Quality Assurance)

   • Added parameter set validation tests in Invoke-DbaDbLogShipping.Tests.ps1:102-121
   • Tests verify FileShare and AzureBlob parameter sets exist
   • Tests confirm SharedPath and AzureBaseUrl are in correct parameter sets

Files Modified

• public/Invoke-DbaDbLogShipping.ps1 - Backtick to splat conversion
• private/functions/New-DbaLogShippingPrimaryDatabase.ps1 - Azure credential validation
• private/functions/New-DbaLogShippingSecondaryPrimary.ps1 - Azure credential validation
• tests/Invoke-DbaDbLogShipping.Tests.ps1 - Added Azure parameter tests

What Was Not Done

The previous review mentioned these as "should fix" or "nice to have" items that weren't critical:

• Normalize Azure URL input (ToLower): Not implemented - Azure storage account names are already lowercase-only by Azure's design, so case normalization isn't necessary
• More verbose logging: The existing verbose logging is adequate for troubleshooting

Testing

The parameter validation test should now pass with the Azure parameters included. The new unit tests validate the parameter set structure to ensure SharedPath and AzureBaseUrl are mutually exclusive.

---

View commit

[potatoqualitee]

lol daaaaaamn FINALLY got it working tho. totally merging.

[claude]

Code Review: Azure Blob Storage Support for Log Shipping

This is a significant feature addition that brings Azure blob storage support to dbatools log shipping. Overall, the implementation is well-structured and follows the project's coding standards. Below are my findings:

✅ Strengths

1. Excellent Feature Design: The Azure blob storage support is a valuable addition that eliminates the need for file share infrastructure and enables cloud-native DR scenarios.

2. Good Backward Compatibility: The changes maintain full backward compatibility with traditional file share log shipping through proper parameter validation.

3. Strong Validation: The Azure URL format validation and credential checking are thorough and will prevent common configuration errors.

4. Comprehensive Testing: The integration test in .github/scripts/gh-actions.ps1 is well-structured with good error handling and cleanup logic.

5. Good Documentation: The parameter help text and examples clearly explain the new Azure functionality.

⚠️ Issues Requiring Attention

Critical Issues

1. CLAUDE.md Style Violations - The PR violates several critical style guidelines from the project's CLAUDE.md:

   Missing Mandatory Parameter Attribute (Invoke-DbaDbLogShipping.ps1:435-437):

   # WRONG - Missing Mandatory attribute
   [parameter(Mandatory)]
   [Alias("BackupNetworkPath")]
   [string]$SharedPath,

   According to CLAUDE.md, SharedPath and AzureBaseUrl are mutually exclusive with exactly one required. The current code only validates this at runtime (line 597-600), but doesn't enforce it at the parameter level. Consider using parameter sets to enforce this at the parameter binding stage.

2. Incomplete Implementation in Invoke-DbaDbLogShipping.ps1: The main function has validation code for Azure (lines 591-627) but appears to be incomplete. The original file shown in the diff had traditional file share validation starting at line 560, but the Azure support modifications don't show the full implementation that actually uses the Azure parameters throughout the function.

   Looking at the diff, I see Azure validation added but the actual Azure-specific logic in the process block appears to be missing from what was shown. The function should:

   • Skip copy job creation when using Azure (since backups go directly to blob storage)
   • Pass Azure credential info to the private functions
   • Handle Azure URLs differently than UNC paths throughout

3. Missing Parameter in Function Signatures: The AzureCredential parameter is added to New-DbaLogShippingPrimaryDatabase and New-DbaLogShippingSecondaryPrimary, but Invoke-DbaDbLogShipping.ps1 never passes it when calling these functions (lines 1509-1523 and 1578-1590). This means the Azure credential validation in the private functions will never work properly.

4. Inconsistent Azure Credential Handling:

   • In New-DbaLogShippingPrimaryDatabase.ps1 (lines 132-180), the function extracts the base container URL from database-specific paths to find the credential.
   • However, if a user provides AzureCredential explicitly, this logic is bypassed - but the parameter is never passed from the calling function.
   • This creates a disconnect where the validation exists but can't be utilized.

Code Quality Issues

5. Missing Parameter Documentation (Invoke-DbaDbLogShipping.ps1):

   • Lines 68-76: Documentation updated for SharedPath and LocalPath mentions Azure exclusivity, but formatting could be improved.
   • Lines 77-85: Good addition of AzureBaseUrl and AzureCredential parameter documentation.
   • However, the author attribution on line 363 says "Sander Stad (@sqlstad), sqlstad.nl" - according to CLAUDE.md rules, when Claude adds significant functionality, it should be listed as "Sander Stad (@sqlstad), sqlstad.nl + Claude (Azure blob storage support)" ✅ (this is actually correct in the code!)

6. Test Coverage Concern (.github/scripts/gh-actions.ps1:275-407):

   • The test is marked with -Skip:(-not $env:azurepasswd) which is good for CI environments.
   • However, the test creates SAS credentials using raw T-SQL (CREATE CREDENTIAL) instead of using New-DbaCredential. While this works, it bypasses the dbatools abstraction layer.
   • The test cleanup is comprehensive (lines 406-434) which is excellent.

7. Regex Pattern Validation (Invoke-DbaDbLogShipping.ps1:594):

   $RegexAzureUrl = '^https?://[a-z0-9]+\.blob\.core\.windows\.net/[a-z0-9\-]+/?'

   This regex might be too restrictive:

   • Storage account names can be up to 24 characters (currently only matches [a-z0-9]+)
   • Container names can contain underscores and other characters
   • Consider: '^https?://[a-z0-9]{3,24}\.blob\.core\.windows\.net/[a-z0-9]([a-z0-9\-]*[a-z0-9])?/?'

8. Error Messages Could Be More Helpful:

   • Line 601 (Invoke-DbaDbLogShipping.ps1): The error message is good but could include a concrete example of the expected format.
   • Lines 146-147 (New-DbaLogShippingPrimaryDatabase.ps1): The error message for missing Azure credential could guide users to use New-DbaCredential with specific examples.

9. Backup-DbaDatabase.ps1 Change (lines 505-507):

   } else {
       # URL is just the container, use it as-is for credential name
       $credentialName = $baseUrl
   }

   This appears to be a bug fix unrelated to log shipping. It should probably be in a separate commit or PR, though it's minor enough that it's acceptable here. Good catch fixing this edge case!

🔧 Recommendations

1. Add Parameter Sets to Invoke-DbaDbLogShipping.ps1 to enforce mutual exclusivity:

   [CmdletBinding(DefaultParameterSetName = "FileShare", SupportsShouldProcess, ConfirmImpact = "Medium")]
   param(
       [Parameter(Mandatory, ParameterSetName = "FileShare")]
       [string]$SharedPath,
       
       [Parameter(ParameterSetName = "FileShare")]
       [string]$LocalPath,
       
       [Parameter(Mandatory, ParameterSetName = "Azure")]
       [string]$AzureBaseUrl,
       
       [Parameter(ParameterSetName = "Azure")]
       [string]$AzureCredential,
       # ... other parameters
   )

2. Pass AzureCredential to Private Functions: Update lines 1509-1523 and 1578-1590 to include the AzureCredential parameter when calling the private functions.

3. Complete the Azure Implementation: The diff appears incomplete. Ensure the process block includes:

   • Logic to skip copy job creation for Azure scenarios
   • Proper handling of Azure URLs vs UNC paths throughout the function
   • Azure-specific backup and restore path handling

4. Improve Regex Validation: Use a more accurate pattern for Azure storage URLs that matches the actual naming constraints.

5. Add More Examples: Consider adding an example showing storage account key authentication (though the comment in the test file correctly notes that SAS tokens are preferred).

6. Parameter Validation Update: If you add parameter sets, update the test file tests/Invoke-DbaDbLogShipping.Tests.ps1 to include the new AzureBaseUrl and AzureCredential parameters in the parameter validation test.

📋 Minor Style Notes

• Line 414: [CmdletBinding(DefaultParameterSetName = "Default", ...)] - if you implement parameter sets, remove the "Default" parameter set name or rename it appropriately.
• Consider adding a .NOTES section comment in the private functions mentioning Azure support was added.
• The test file has good debug output (lines 318-331) that will help troubleshoot CI failures.

✨ Conclusion

This is a valuable feature that's mostly well-implemented. The main concerns are:

1. Complete the implementation - ensure Azure parameters are passed through the call chain
2. Add parameter sets for better parameter validation
3. Verify the diff is complete - some Azure logic appears to be missing from the process block

Once these issues are addressed, this will be an excellent addition to dbatools. The test coverage is solid, the documentation is clear, and the backward compatibility is maintained.

Recommendation: Request changes to address the critical issues (missing parameter passing and incomplete implementation), then approve after those are fixed.

---

Review generated by Claude Code following dbatools CLAUDE.md style guidelines

[potatoqualitee]

@claude add a short note in CLAUDE.md that we avoid ParameterSets because their error messages are terrible and hard to use. We use Test-Bound instead and give the users useful information. (dont add a whole section about this, just a single line is fine)

also a SHORT NOTE that there should not be extra line breaks after parametrs
BAD:

    [Parameter(Mandatory, ParameterSetName = "FileShare")]
    [string]$SharedPath,
    
    [Parameter(ParameterSetName = "FileShare")]
    [string]$LocalPath,

GOOD:

    [Parameter(Mandatory, ParameterSetName = "FileShare")]
    [string]$SharedPath,
    [Parameter(ParameterSetName = "FileShare")]
    [string]$LocalPath,

also pls do improve the regex as suggested

also add a a single line to give concrete examples in the error messages and add them to this command too, thank you.

add in the notes about azure, sure

Also, tell me in depth what you meant here?

Pass AzureCredential to Private Functions: Update lines 1509-1523 and 1578-1590 to include the AzureCredential parameter when calling the private functions.

Complete the Azure Implementation: The diff appears incomplete. Ensure the process block includes:

Logic to skip copy job creation for Azure scenarios
Proper handling of Azure URLs vs UNC paths throughout the function
Azure-specific backup and restore path handling

it tested well, are you sure?

[claude]

Claude finished @potatoqualitee's task —— View job

---

Summary

I've completed all the requested updates. Here's what was done:

Changes Made

1. CLAUDE.md Updates

   • Added note about avoiding ParameterSets (their error messages are terrible; use Test-Bound with helpful messages instead)
   • Added note about no extra line breaks between parameter declarations

2. Improved Azure URL Regex

   • Updated from [a-z0-9]+ to [a-z0-9]{3,24} for storage account names (Azure's actual constraint)
   • Updated container name pattern to [a-z0-9]([a-z0-9\-]*[a-z0-9])? to properly handle hyphens
   • Applied to all three files: Invoke-DbaDbLogShipping.ps1, New-DbaLogShippingPrimaryDatabase.ps1, New-DbaLogShippingSecondaryPrimary.ps1

3. Concrete Examples in Error Messages

   • All Azure URL format error messages now include: (example: https://mystorageaccount.blob.core.windows.net/logshipping)
   • Applied to all validation points in the three functions

4. Azure Support Attribution

   • Updated .NOTES sections in both private functions to credit Azure support: "Azure blob storage support added by Claude"

Clarification on Previous Review Comments

You asked about my earlier review comments regarding AzureCredential passing and Azure implementation completeness. After reviewing the actual code:

I was incorrect in my previous review. Here's what I found:

1. AzureCredential IS being passed correctly:

   • Line 1686-1688 in Invoke-DbaDbLogShipping.ps1: if ($AzureCredential) { $splatPrimary.AzureCredential = $AzureCredential }
   • Line 1767-1770: if ($AzureCredential) { $splatSecondaryPrimary.AzureCredential = $AzureCredential }
   • The parameter is conditionally added to the splat before calling the private functions

2. Azure implementation IS complete:

   • Lines 609-631: Azure URL validation and version checking
   • Lines 629-631: Azure URLs set for both SharedPath and LocalPath
   • Line 1775-1783: Copy job is removed for Azure scenarios (backups go directly to blob storage)
   • The implementation properly handles Azure URLs vs UNC paths throughout

Why I made the mistake: I was looking at a partial diff view and didn't see the conditional splat parameter additions. The actual implementation tested well because it's correct!

The code is solid and working as designed. My apologies for the confusion in the initial review.

---

Commit: 832d4de