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

SqlDatabaseUser: Pass parameter ServerName to Assert-SqlLogin #1681

Merged
merged 6 commits into from
Feb 1, 2021

Conversation

bschapendonk
Copy link
Contributor

@bschapendonk bschapendonk commented Jan 29, 2021

Added -ServerName to Assert-SqlLogin. @PSBoundParameters doesn't capture $ServerName when it is not explicitly set by the caller.

Fixes #1647


This change is Reviewable

@codecov
Copy link

codecov bot commented Jan 29, 2021

Codecov Report

Merging #1681 (41489f8) into main (db3c76f) will increase coverage by 0%.
The diff coverage is 100%.

Impacted file tree graph

@@         Coverage Diff          @@
##           main   #1681   +/-   ##
====================================
  Coverage    97%     97%           
====================================
  Files        38      38           
  Lines      6260    6268    +8     
====================================
+ Hits       6103    6112    +9     
+ Misses      157     156    -1     
Flag Coverage Δ
unit 97% <100%> (+<1%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...urces/DSC_SqlDatabaseUser/DSC_SqlDatabaseUser.psm1 98% <100%> (+<1%) ⬆️

@johlju johlju changed the title ResourceName: SqlDatabaseUser Added -ServerName to Assert-SqlLogin issue #1647 SqlDatabaseUser: Added -ServerName to Assert-SqlLogin issue #1647 Jan 30, 2021
@johlju johlju added the needs review The pull request needs a code review. label Jan 30, 2021
Copy link
Member

@johlju johlju left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 2 of 2 files at r1, 1 of 1 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@johlju
Copy link
Member

johlju commented Jan 30, 2021

@bschapendonk great work finding this issue! Thank you very much! 🙇
I pushed regression tests for this issue (that will fail without your change) so we don't get a regression in this later.

@johlju johlju added ready for merge The pull request was approved by the community and is ready to be merged by a maintainer. and removed needs review The pull request needs a code review. labels Jan 30, 2021
Copy link
Member

@johlju johlju left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r3.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@johlju johlju changed the title SqlDatabaseUser: Added -ServerName to Assert-SqlLogin issue #1647 SqlDatabaseUser: Pass parameter ServerName to Assert-SqlLogin Jan 30, 2021
@johlju
Copy link
Member

johlju commented Jan 31, 2021

The integration test are failing with the following error: Cannot bind parameter because parameter 'ServerName' is specified more than once.

VERBOSE: [fv-az49-982]: LCM:  [ End    Set      ]  [[SqlDatabaseUser]Integration_Test_DatabaseUser]  in 0.3590 seconds.
      [-] Should compile and apply the MOF without throwing 7.05s
        Expected no exception to be thrown, but an exception "PowerShell DSC resource DSC_SqlDatabaseUser  failed to execute Set-TargetResource functionality with error message: System.InvalidOperationException: Failed creating the database user 'DscUser4' in the database 'DefaultDb' with the user type 'Login'. (SDU0013) ---> System.Management.Automation.ParameterBindingException: Cannot bind parameter because parameter 'ServerName' is specified more than once. To provide multiple values to parameters that can accept multiple values, use the array syntax. For example, "-parameter value1,value2,value3".
           at System.Management.Automation.ExceptionHandlingOps.CheckActionPreference(FunctionContext funcContext, Exception exception)
           at System.Management.Automation.Interpreter.ActionCallInstruction`2.Run(InterpretedFrame frame)
           at System.Management.Automation.Interpreter.EnterTryCatchFinallyInstruction.Run(InterpretedFrame frame)
           at System.Management.Automation.Interpreter.EnterTryCatchFinallyInstruction.Run(InterpretedFrame frame)
           --- End of inner exception stack trace --- " was thrown from D:\a\1\s\tests\Integration\DSC_SqlLogin.Integration.Tests.ps1:247 char:21
            + ...               Start-DscConfiguration @startDscConfigurationParameters
            +                   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~.
        248:                 } | Should -Not -Throw
        at <ScriptBlock>, D:\a\1\s\tests\Integration\DSC_SqlLogin.Integration.Tests.ps1: line 229

@johlju
Copy link
Member

johlju commented Jan 31, 2021

@bschapendonk running this example works on PowerShell 7, but does not work in Windows PowerShell. The unit tests runs in PowerShell 7 that way the above issue is not caught, but caught in integration tests as those are run in Windows PowerShell.

function Assert-SqlLogin
{
    [CmdletBinding()]
    param
    (
        [Parameter(Mandatory = $true)]
        [System.String]
        $ServerName,

        [Parameter(Mandatory = $true)]
        [System.String]
        $InstanceName,

        [Parameter(Mandatory = $true)]
        [System.String]
        $LoginName,

        # Catch all other splatted parameters from $PSBoundParameters
        [Parameter(ValueFromRemainingArguments = $true)]
        $RemainingArguments
    )

    Write-Verbose $ServerName -Verbose
    # Skipped code for example
}

function Set-TargetResource
{
    [CmdletBinding()]
    param
    (
        [Parameter()]
        [ValidateNotNullOrEmpty()]
        [System.String]
        $ServerName = $env:COMPUTERNAME,

        [Parameter(Mandatory = $true)]
        [System.String]
        $InstanceName,

        [Parameter()]
        [ValidateNotNullOrEmpty()]
        [System.String]
        $LoginName
    )

    Assert-SqlLogin -ServerName $ServerName @PSBoundParameters
}

Set-TargetResource -LoginName 'kalle' -InstanceName 'SQL2019'
Set-TargetResource -LoginName 'kalle' -InstanceName 'SQL2019' -ServerName 'host.company.local'

@johlju johlju self-requested a review January 31, 2021 10:01
Copy link
Member

@johlju johlju left a comment

Choose a reason for hiding this comment

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

Reviewable status: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @bschapendonk and @johlju)


source/DSCResources/DSC_SqlDatabaseUser/DSC_SqlDatabaseUser.psm1, line 246 at r4 (raw file):

Assert-SqlLogin -ServerName $ServerName @PSBoundParameters

I think we need to to do this instead as the change is not supported by Windows PoweerShell. What do you think?

$assertSqlLoginParameters = @{} + $PSBoundParameter
$assertSqlLoginParameters['ServerName'] = $ServerName

Assert-SqlLogin @assertSqlLoginParameters 

Alternative is to just pass the required values.

$assertSqlLoginParameters = @{
    ServerName = $ServerName
    InstanceName = $InstanceName
    LoginName = $LoginName
}


Assert-SqlLogin @assertSqlLoginParameters 

source/DSCResources/DSC_SqlDatabaseUser/DSC_SqlDatabaseUser.psm1, line 363 at r4 (raw file):

Assert-SqlLogin -ServerName $ServerName @PSBoundParameters

Same as previous comment.

@johlju johlju added waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. and removed ready for merge The pull request was approved by the community and is ready to be merged by a maintainer. labels Jan 31, 2021
@johlju
Copy link
Member

johlju commented Jan 31, 2021

@bschapendonk I will push a change for my comment so we can get this fix into the new release today.

bschapendonk and others added 5 commits January 31, 2021 16:51
… commands in an interactive mode dsccommunity#1647

$servername is not passed thru @PSBoundParameters because the default is used and it is not explicitly set by the caller
Copy link
Member

@johlju johlju left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 3 of 3 files at r6.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @bschapendonk)

@johlju johlju merged commit f7676b0 into dsccommunity:main Feb 1, 2021
@johlju
Copy link
Member

johlju commented Feb 1, 2021

@bschapendonk awesome work finding this issue and providing a fix! Much appreciated!

@johlju johlju removed the waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. label Feb 1, 2021
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.

SqlDatabaseUser: PowerShell DSC does not support execution of commands in an interactive mode
2 participants