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

New command for adding a linked server #7392

Merged
merged 2 commits into from
Jun 18, 2021
Merged

New command for adding a linked server #7392

merged 2 commits into from
Jun 18, 2021

Conversation

lancasteradam
Copy link
Contributor

@lancasteradam lancasteradam commented May 31, 2021

Type of Change

  • Bug fix (non-breaking change, fixes # )
  • New feature (non-breaking change, adds functionality, fixes Commands for managing Linked Server #6506 )
  • Breaking change (effects multiple commands or functionality, fixes # )
  • 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

Basic command to add a new linked server.

Commands to test

See the integration tests.

@andreasjordan
Copy link
Contributor

So this will be part of #6506?

@lancasteradam
Copy link
Contributor Author

Hi @andreasjordan, I had not seen that issue, but yes I will update the description of this PR and the other one I just added for removing a linked server so that they both reference 6506.

Thanks,
Adam

@andreasjordan
Copy link
Contributor

There is also this one: #6196

@lancasteradam
Copy link
Contributor Author

When I tried the "set" workflow SMO displayed an error message indicating that the linked server has to be dropped and recreated.

@andreasjordan
Copy link
Contributor

Ok, then maybe you can add the messages to the feature request and close it.

@potatoqualitee
Copy link
Member

Looks nice! Any chance you can add a LoginCredential or something similarly named and allow them to create a Linked Server login?

image

If it's too much work, we can add it later.

@lancasteradam
Copy link
Contributor Author

@potatoqualitee, would it be ok to accept the current scope of this PR as a phase 1? I could come back to the login piece at a later date as a phase 2. I would want to proceed very carefully on that part because of the need for secure credential handling.

@andreasjordan, I added the comment to #6196, but I don't have permission to close issues.

}

$newLinkedServer.Create()
$newLinkedServer
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use Get-DbaLinkedServer to get a nice object with the custom properties for the new linked server:

$instance |  Get-DbaLinkedServer -LinkedServer $LinkedServer

$InputObject += Connect-DbaInstance -SqlInstance $instance -SqlCredential $SqlCredential
}

foreach ($instance in $InputObject) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If something is a server SMO with an active connection, so the output of Connect-DbaInstance, we call this $server. And there are definitly server SMOs in $InputObject so I would change this line to

foreach ($server in $SqlInstance) {

@potatoqualitee
Copy link
Member

That will work, @lancasteradam ! Happy to merge this PR and then you can add later, once @andreasjordan's review has been addressed.

- Use Get-DbaLinkederver to return the new linked server
- Use the $server name convention instead of $instance
@lancasteradam
Copy link
Contributor Author

Thank you @potatoqualitee and @andreasjordan. I've incorporated the feedback mentioned by @andreasjordan.

@andreasjordan
Copy link
Contributor

Approved both pull request.

@potatoqualitee potatoqualitee merged commit c28dbd7 into dataplat:development Jun 18, 2021
@potatoqualitee
Copy link
Member

Fantastic 💯 !

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.

Commands for managing Linked Server
3 participants