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

Standardize Remove-DbaDatabaseSafely #5618

Merged

Conversation

ClaudioESSilva
Copy link
Collaborator

Type of Change

  • Bug fix (non-breaking change, fixes #)
  • New feature (non-breaking change, adds functionality)
  • Breaking change (effects multiple commands or functionality)
  • Ran manual Pester test and has passed (`.\tests\manual.pester.ps1)
  • Adding code coverage to existing functionality
  • Pester test is included
  • If new file reference added for test, has is been added to github.com/sqlcollaborative/appveyor-lab ?
  • Nunit test is included
  • Documentation
  • Build system

Purpose

Standardize Remove-DbaDatabaseSafely function.

Approach

Commands to test

Screenshots

Learning

@ClaudioESSilva ClaudioESSilva self-assigned this May 27, 2019
@ClaudioESSilva ClaudioESSilva requested a review from wsmelton May 27, 2019 22:54
@ClaudioESSilva
Copy link
Collaborator Author

Can someone take a look?

Regarding the tests, I would prefer not to add the -Skip but this is annoying..

image
It seems that the final dbcc checkdb is "skipped" and the output isn't returned.

When running manually it works:
image

Another appveyor craziness?

@potatoqualitee
Copy link
Member

thanks for tackling this, claudio! I took a look and see nothing off 👎 This may be a skipper. if you'd like, you can also login to appveyor to see whats goingon.

}
process {
if (Test-FunctionInterrupt) {
return
}
try {
Start-SqlAgent
$destInstanceName = $destserver.InstanceName
if ($destserver.InstanceName -eq '') {
Copy link
Member

Choose a reason for hiding this comment

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

This should include test for Edition because on Express this will cause the command to fail.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks!
Done.


if ($force -and $dbccgood -ne "Success") {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if ($force -and $dbccgood -ne "Success") {
if ($Force -and $dbccgood -ne "Success") {

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks!
Done.

if ($DefaultCompression -eq 1) {
$null = Backup-DbaDatabase -SqlInstance $sqlinstance -SqlCredential $SqlCredential -Database $dbname -BackupFileName $filename -CompressBackup -Checksum
} else {
$null = Backup-DbaDatabase -SqlInstance $sqlinstance -SqlCredential $SqlCredential -Database $dbname -BackupFileName $filename -Checksum
Copy link
Member

Choose a reason for hiding this comment

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

Use of splatting will allow you to take this down to just calling the backup command once...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks!
Done.

@potatoqualitee
Copy link
Member

thank you so much shawn 💯

Thanks Shawn!
-WithReplace needed because database will only be dropped after job creation.
Restore-DbaDatabase command uses Test-DbaBackupInformation which fails if database already exists
Create another database for the test between.
Change expected warning message for Express Edition
Last test should run between on instance2 and instance3.
Should be #script:instance2 and $script:instance3
Should be #script:instance2 and $script:instance3
Fix order of source & destination instances
Reorder instances.
Backup from 2016 test on 2017
@ClaudioESSilva
Copy link
Collaborator Author

ClaudioESSilva commented May 29, 2019

Locally is working 100%!
image

Don't know why it stills yielding the error 😭

image

https://ci.appveyor.com/project/sqlcollaborative/dbatools/builds/24908965/job/4uq0kjt5gjw27502

@wsmelton
Copy link
Member

What version of Pester are you using on your local machine?

@wsmelton
Copy link
Member

@wsmelton
Copy link
Member

(This will obviously change, but not for 1.0 release)

@potatoqualitee
Copy link
Member

i had this issue the other day and cant remember how i fixed it 😭

@ClaudioESSilva
Copy link
Collaborator Author

@wsmelton was running with 4.5.0
Tested with 4.4.2 and it works

image

Put context block back to last position
@wsmelton
Copy link
Member

It fails for me

image

@wsmelton
Copy link
Member

Was on the branch for that last one but changed to prerelease and I get the same error with the current version.

@wsmelton
Copy link
Member

wsmelton commented May 30, 2019

It passed....quick merge it now @potatoqualitee

giphy

@ClaudioESSilva
Copy link
Collaborator Author

Not yet..
https://ci.appveyor.com/project/sqlcollaborative/dbatools/builds/24909596/job/xctqpq961eu0hih5
It has 'passed' before but the error is there.
@wsmelton on your test you have defined 3 different instances? Or repeated any on the 3 variables?

@potatoqualitee
Copy link
Member

I forgot! I never got this to work! It was Find-DbaAgent. So I'll merge this, disable the test and revisit later 💯

@potatoqualitee potatoqualitee merged commit 59a9e3d into prerelease May 30, 2019
@potatoqualitee potatoqualitee deleted the RewriteAndStandardize_Remove-DbaDatabaseSafely branch May 30, 2019 09:43
@potatoqualitee
Copy link
Member

THANK YOU BOTH!!!! 🎉

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.

3 participants