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

Restore-DbaDatabase - Changed WhatIf message / added deep copy of FileList #7410

Merged
merged 10 commits into from
Jun 21, 2021

Conversation

andreasjordan
Copy link
Contributor

@andreasjordan andreasjordan commented Jun 5, 2021

Type of Change

@andreasjordan
Copy link
Contributor Author

image

@andreasjordan andreasjordan changed the title Restore-DbaDatabase - Changed WhatIf message Restore-DbaDatabase - Changed WhatIf message / added deep copy of FileList Jun 5, 2021
@andreasjordan
Copy link
Contributor Author

Not sure if the deep copy has some side effects. Will have to look at that.

@andreasjordan
Copy link
Contributor Author

Ok, deep copy breaks Test-DbaLastBackup, more work has to be done...

@potatoqualitee
Copy link
Member

Hey @Stuart-Moore, can you please review this PR? 🙏🏼

@potatoqualitee
Copy link
Member

Will reach out to Stuart on Twitter, this command is ultra important so I want to ensure he gets a look.

@Stuart-Moore
Copy link
Contributor

Does the copy not want to be a bit further into the function? All the ways of passing the backup info into the function end up in BackupHistory objects, so should all be affected? Somewhere like line 619 in your modified file

Not played with this for a long time and have been rotting my brain with ADF and Dynamics so may have missed something.

The overall way of doing it should work.

would be nice to see a test adding, just to make sure someone else doesn't break it later on by mistake or by misunderstanding.

@andreasjordan
Copy link
Contributor Author

Output of the test, if solution is not active:

image

@andreasjordan
Copy link
Contributor Author

Hi @Stuart-Moore , I had another look at this and found a better solution. I realized that you do a lot of $f = $f | Select-Object *, ... to get a new object with only that values from the piped in backup history. But as the FileList property is an array of objects, the new backup history object just references the same FileList objects. To implement a "deep copy" (is this really the correct word?) I just used the same technique as you did with the backup history object.
I added a test, you find a screenshot of the failing test a comment above.

@@ -677,7 +679,7 @@ function Restore-DbaDatabase {
return
}
$pathSep = Get-DbaPathSep -Server $RestoreInstance
$null = $BackupHistory | Format-DbaBackupInformation -DataFileDirectory $DestinationDataDirectory -LogFileDirectory $DestinationLogDirectory -DestinationFileStreamDirectory $DestinationFileStreamDirectory -DatabaseFileSuffix $DestinationFileSuffix -DatabaseFilePrefix $DestinationFilePrefix -DatabaseNamePrefix $RestoredDatabaseNamePrefix -ReplaceDatabaseName $DatabaseName -Continue:$Continue -ReplaceDbNameInFile:$ReplaceDbNameInFile -FileMapping $FileMapping -PathSep $pathSep
$BackupHistory = $BackupHistory | Format-DbaBackupInformation -DataFileDirectory $DestinationDataDirectory -LogFileDirectory $DestinationLogDirectory -DestinationFileStreamDirectory $DestinationFileStreamDirectory -DatabaseFileSuffix $DestinationFileSuffix -DatabaseFilePrefix $DestinationFilePrefix -DatabaseNamePrefix $RestoredDatabaseNamePrefix -ReplaceDatabaseName $DatabaseName -Continue:$Continue -ReplaceDbNameInFile:$ReplaceDbNameInFile -FileMapping $FileMapping -PathSep $pathSep
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good to me

$BackupHistory += $F | Select-Object *, @{ Name = "ServerName"; Expression = { $_.SqlInstance } }, @{ Name = "BackupStartDate"; Expression = { $_.Start -as [DateTime] } }
# Fix #5036 by implementing a deep copy of the FileList
$f.FileList = $f.FileList | Select-Object *
$BackupHistory += $f | Select-Object *, @{ Name = "ServerName"; Expression = { $_.SqlInstance } }, @{ Name = "BackupStartDate"; Expression = { $_.Start -as [DateTime] } }
Copy link
Contributor

Choose a reason for hiding this comment

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

This should do it.

@@ -143,6 +143,16 @@ Describe "$CommandName Integration Tests" -Tag "IntegrationTests" {
}
}

Context "Replace databasename in Restored File, but don't change backup history #5036" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Tests are good 👍

Should stop your hard work getting broken further on down the line

@Stuart-Moore
Copy link
Contributor

Happy with all of that. Merging in the new changes from dev to make sure all is still good, then i'll merge it in properly. If I get distracted, anyone else who wants to merge it, please do.

@Stuart-Moore Stuart-Moore merged commit 2abe567 into development Jun 21, 2021
@Stuart-Moore
Copy link
Contributor

Squashed and merged

Nice work @andreasjordan

@andreasjordan andreasjordan deleted the 6662_RestoreDbaDatabase_WhatIf branch June 21, 2021 06:42
@potatoqualitee
Copy link
Member

Thank you all! 🙇🏼 I'll release this hopefully within the next 48 hours. I'm working on an auto releaser and happy to have something in the queue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants