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

Adding multi-threaded execution for Update-DbaInstance #4775

Merged
merged 13 commits into from Dec 6, 2018

Conversation

Projects
None yet
2 participants
@nvarscar
Contributor

nvarscar commented Dec 5, 2018

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
  • Nunit test is included
  • Documentation
  • Build system

Purpose

Adding multithreaded invocation of SQL Server patches following proper flow.

Approach

Moving around pieces of code, improving messaging and using Invoke-Parallel when computer count -ge 2

Commands to test

update-dbainstance computer1,computer2,computerN -path '...' -credential $cred

Screenshots

image

image

image

Learning

Handling warnings from inside a runspace'd scriptblock is a pain

@potatoqualitee

This comment has been minimized.

Member

potatoqualitee commented Dec 5, 2018

i am so incredibly confused as to why these files keep getting merge conflicts 👎 let me see what happens when we try straight to master.

@potatoqualitee potatoqualitee changed the base branch from development to master Dec 5, 2018

@potatoqualitee potatoqualitee changed the base branch from master to development Dec 5, 2018

@nvarscar

This comment has been minimized.

Contributor

nvarscar commented Dec 5, 2018

it might be because I used my old branch without merging development back. I was hoping that no commits would get in the way, but apparently some did.

nvarscar added some commits Dec 5, 2018

@potatoqualitee

This comment has been minimized.

Member

potatoqualitee commented Dec 6, 2018

ohh snap! we got green 💃

@potatoqualitee

This comment has been minimized.

Member

potatoqualitee commented Dec 6, 2018

Thanks so much for the updates.

can you do the remotehost/credential check earlier on? it essentially takes 15 seconds to tell me that I did something wrong and I'll have to do it all over again.

image

I believe that last one should be a Stop-Function, especially if you're looking for $_.Message to exist.

@potatoqualitee

This comment has been minimized.

Member

potatoqualitee commented Dec 6, 2018

Is the failback prompt going to be done later? I forgot what we had talked about.

image

Also, can you change the wording on that warning? Change cmdlet to command. cmdlets are reserved for C# PowerShell commands. June Blender suggested using command to cover both cmdlets and functions (which is what we use)

And one more change! I named that column wrong. Can you do Notes instead of Message to align with the rest of our commands? 🙇

@nvarscar

This comment has been minimized.

Contributor

nvarscar commented Dec 6, 2018

can you do the remotehost/credential check earlier on? it essentially takes 15 seconds to tell me that I did something wrong and I'll have to do it all over again.

Without breaking yet another whatif test - only as a warning without stopping a function. Whatif runs do not require credentials. Hope that works.

image

I believe that last one should be a Stop-Function, especially if you're looking for $_.Message to exist.

Fixed - with the exception that it was intended specifically as a warning, since there is no way we're getting there with -EnableExceptions when there are errors - and I don't want the output processing scriptblock to be throwing.

Is the failback prompt going to be done later? I forgot what we had talked about.

It's not a failback. It's running the command without elevated session, when it tries to configure CredSSP first. I don't want to enforce an elevated session and exit right away, since, theoretically, it might work even without the elevated session. But it's required for setting up a connection to a remote host for the first time. I would rather let it be and let the users know that they are supposed to run it in an elevated session.

Also, can you change the wording on that warning? Change cmdlet to command. cmdlets are reserved for C# PowerShell commands. June Blender suggested using command to cover both cmdlets and functions (which is what we use)

The warning actually comes from a cmdlet - I'm just including it into the message.

And one more change! I named that column wrong. Can you do Notes instead of Message to align with the rest of our commands? 🙇

Done

@potatoqualitee

This comment has been minimized.

Member

potatoqualitee commented Dec 6, 2018

fantastic! thanks so much!

@potatoqualitee potatoqualitee merged commit 34a7d7f into sqlcollaborative:development Dec 6, 2018

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@potatoqualitee

This comment has been minimized.

Member

potatoqualitee commented Dec 6, 2018

🍌 💃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment