New-DbaDbUser: Refactor and added contained user creation #8827
Conversation
|
Please use the template provided for the pull request when it deals with modifying code. It helps in review of the PR. |
| } | ||
|
|
||
| #Set appropriate user type | ||
| #Removed SQLLogin user type. This is deprecated and the alternate is to map to SqlUser. Reference https://learn.microsoft.com/en-us/dotnet/api/microsoft.sqlserver.management.smo.usertype?view=sql-smo-160 |
There was a problem hiding this comment.
Your link does not show that is deprecated. This will still work on lower versions of SQL Server?
There was a problem hiding this comment.
It is mentioned in the fields section. https://learn.microsoft.com/en-us/dotnet/api/microsoft.sqlserver.management.smo.usertype?view=sql-smo-160#fields
SqlLogin 0 Specifies a SQLLogin user. This is deprecated, use SQLUser instead.
I might be wrong. I thought the smo library is part of dbatools or dbatools.library module as a dependency which should be the recent version. If this is the case then I will prefer to keep the changes. However, if the module uses smo library installed locally then it makes sense to revert it back. I was not clear, your inputs will be helpful.
| if ($ExcludeDatabase) { | ||
| $databases = $databases | Where-Object Name -NotIn $ExcludeDatabase | ||
| } | ||
| $databases = Get-DbaDatabase @getDbParam |
There was a problem hiding this comment.
We tend to not use this in our commands internally because of performance impact. I would prefer this to be reverted to the original pattern as that is what is followed in most all of our commands.
There was a problem hiding this comment.
Sorry I didnt get it. You meant the hash table as parameters or using the Get-DbaDatabase function?
There was a problem hiding this comment.
My request is reverting the change back to the original pattern. Get-DbaDatabase incurs performance on many systems and use of that in any command can cause degradation.
There was a problem hiding this comment.
Gotcha. Before making this change I raised in the Slack channel and I was told there isn't any standard and suggested to use functions if there is one already exists. That's when I refactored to use the functions instead.
I'm happy to revert the changes to use SMO connection instead of the function. Since this PR is merged, do you guys want me to revert it and raise a new PR?
Also, this is my understanding. These are operational functions and not a real time application so it should be fine even if it add few ms to the execution. I know this module is used in wide scope so I dont want to mess up with the expectation. Like I said before, let me know if I need to revert it and create a new PR?
|
The test failures are related to our tests use |
|
hey cool! thank you for the PR. I plan to return to dbatools this weekend and will take a closer look as well. |
Fixed the failing tests and all the tests are successful now. |
Thanks for this, updated the description using the template. My first PR with dbatools so please bear with me till I get used to it. |
public/New-DbaDbUser.ps1
Outdated
| $connParam = @{ } | ||
| if ($SqlCredential) { $connParam.SqlCredential = $SqlCredential } | ||
|
|
||
| # is this required? |
There was a problem hiding this comment.
Please remove comment, it is required.
There was a problem hiding this comment.
shame! I kept that comment. Apologies.
Co-authored-by: Shawn Melton <11204251+wsmelton@users.noreply.github.com>
Co-authored-by: Shawn Melton <11204251+wsmelton@users.noreply.github.com>
Co-authored-by: Shawn Melton <11204251+wsmelton@users.noreply.github.com>
|
Thank you for the proposed changes, @wsmelton 🙇🏼 I have merged them. |
|
sweet, the merges ultimately resulted in a pass. |
Type of Change
.\tests\manual.pester.ps1)Purpose
dbatoolspublic functions.SqlLoginuser typeApproach
Included a new parameter (Password) to the function and also used parameter sets. Depends on which parameter the user choose then action will be taken. In this case if they use
Passwordparameter then it will create a SQL user with password in the database. SQL User with password is supported only in contained database and from SQL 2012 onwards. This check is not done , if the database is not enabled partial containment then at the time of execution a error will be thrown.Commands to test
New-DbaDbUser -SqlInstance sqlserver2016 -Database DB1 -Username user1 -Password (ConvertTo-SecureString -String "DBATools" -AsPlainText)