-
Notifications
You must be signed in to change notification settings - Fork 82
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
BREAKING CHANGE: PSResourceRepository: Resource to manage PowerShell Package Repositories #395
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 20 files reviewed, 1 unresolved discussion (waiting on @nickgw)
tests/Unit/DSC_Computer.Tests.ps1
line 1407 at r2 (raw file):
-Credential $credential` -Verbose } | Should -Throw $message
I think removing this block has broken the tests. Was that intentional?
Codecov Report
@@ Coverage Diff @@
## main #395 +/- ##
====================================
- Coverage 90% 89% -2%
====================================
Files 17 18 +1
Lines 1726 1940 +214
====================================
+ Hits 1564 1730 +166
- Misses 162 210 +48
|
Thanks, don't know how that made it in! |
…uterManagementDsc into nickg_psresource_393
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 21 of 28 files reviewed, 10 unresolved discussions (waiting on @johlju and @PlagueHO)
source/Classes/020.PSResourceRepository.ps1
line 226 at r22 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
I think we should have kept this code, it feels cleaner too. See other comment.
Done.
I gonna have a few busy days at work. But after friday I'm off work (vacation) for a month so will continue the review this weekend. 🙂 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gone through all the files now. This should hopefully be the last of the comments, or very close to it at least 🙂
Reviewed 1 of 4 files at r30, 1 of 1 files at r42, 1 of 3 files at r43, 3 of 3 files at r44, 3 of 3 files at r45, all commit messages.
Reviewable status: all files reviewed, 27 unresolved discussions (waiting on @nickgw and @PlagueHO)
source/Classes/020.PSResourceRepository.ps1
line 181 at r45 (raw file):
#* The user may have specified Proxy & Proxy Credential, or InstallationPolicy params Set-PSRepository @params
We can remove the blank line her
source/Classes/020.PSResourceRepository.ps1
line 241 at r45 (raw file):
} hidden [System.Collections.Hashtable] GetCurrentState1 ([System.Collections.Hashtable] $properties)
We can now remove this function.
Code quote:
GetCurrentState1
source/en-US/PSResourceRepository.strings.psd1
line 18 at r45 (raw file):
NoDefaultSettingsPSGallery = The parameter Default must be set to True for a repository named PSGallery. DefaultSettingsNoPSGallery = The parameter Default may only be used with repositories named PSGallery. CurrentState = Repository '{0}' property '{1}' current state is '{2}'.
When the function is removed as per previous comment, I think we can remove this localization string also.
Code quote:
CurrentState
tests/Unit/Classes/PSResourceRepository.Tests.ps1
line 65 at r45 (raw file):
Describe 'PSResourceRepository\Get()' -Tag 'Get' {
We can remove this blank line
tests/Unit/Classes/PSResourceRepository.Tests.ps1
line 69 at r45 (raw file):
Context 'When the repository is Present with default values' { It 'Should return the correct result when the Repository is present and default params are passed' {
We can remove this blank line
tests/Unit/Classes/PSResourceRepository.Tests.ps1
line 85 at r45 (raw file):
Add-Member -Force -MemberType 'ScriptMethod' -Name 'GetCurrentState' -Value { return [System.Collections.Hashtable] @{ Name = 'FakePSGallery'
We should indent this one step to show it is part of the pipeline that is started on the previous line. Throughout the test code on similiar code.
Code quote:
Add-Member -Force -MemberType 'ScriptMethod' -Name 'GetCurrentState' -Value {
return [System.Collections.Hashtable] @{
Name = 'FakePSGallery'
SourceLocation = 'https://www.powershellgallery.com/api/v2'
Ensure = 'Present'
InstallationPolicy = 'Untrusted'
PackageManagementProvider = 'Nuget'
}
} -PassThru |
Add-Member -Force -MemberType 'ScriptMethod' -Name 'AssertProperties' -Value {
return
}
tests/Unit/Classes/PSResourceRepository.Tests.ps1
line 96 at r45 (raw file):
} $currentState = $script:mockPSResourceRepositoryInstance.Get()
Missing the evaluation of the properties Credential
, Proxy
, ProxyCredential
, and Default
so they return the expected value ($null
?).
Code quote:
$currentState = $script:mockPSResourceRepositoryInstance.Get()
tests/Unit/Classes/PSResourceRepository.Tests.ps1
line 138 at r45 (raw file):
} $currentState = $script:mockPSResourceRepositoryInstance.Get()
Missing the evaluation of the properties Credential
, Proxy
, ProxyCredential
, and Default
so they return the expected value ($null
?).
Code quote:
$currentState = $script:mockPSResourceRepositoryInstance.Get()
tests/Unit/Classes/PSResourceRepository.Tests.ps1
line 168 at r45 (raw file):
return } $currentState = $script:mockPSResourceRepositoryInstance.Get()
Missing the evaluation of the properties Credential
, Proxy
, ProxyCredential
, and Default
so they return the expected value ($null
?).
Code quote:
$currentState = $script:mockPSResourceRepositoryInstance.Get()
tests/Unit/Classes/PSResourceRepository.Tests.ps1
line 184 at r45 (raw file):
Context 'When the repository is present but should be absent' { It 'Should return the correct result when the Repository is present but should be absent' {
We can remove this blank line
tests/Unit/Classes/PSResourceRepository.Tests.ps1
line 188 at r45 (raw file):
$script:mockPSResourceRepositoryInstance = [PSResourceRepository] @{ Name = 'FakePSGallery' SourceLocation = 'https://www.powershellgallery.com/api/v2'
We don't need to provide this when the configuration should be absent.
Code quote:
SourceLocation = 'https://www.powershellgallery.com/api/v2'
tests/Unit/Classes/PSResourceRepository.Tests.ps1
line 235 at r45 (raw file):
Ensure = 'Absent' SourceLocation = 'https://www.powershellgallery.com/api/v2' InstallationPolicy = 'Untrusted'
These three should probably return $null since we are mocking the repository to be absent?
Code quote:
SourceLocation = 'https://www.powershellgallery.com/api/v2'
InstallationPolicy = 'Untrusted'
PackageManagementProvider = 'NuGet'
tests/Unit/Classes/PSResourceRepository.Tests.ps1
line 302 at r45 (raw file):
$script:mockPSResourceRepositoryInstance = [PSResourceRepository] @{ Name = 'FakePSGallery' SourceLocation = 'https://www.powershellgallery.com/api/v2'
We don't need to provide this when the configuration should be absent.
Code quote:
SourceLocation = 'https://www.powershellgallery.com/api/v2'
tests/Unit/Classes/PSResourceRepository.Tests.ps1
line 402 at r45 (raw file):
InModuleScope -ScriptBlock { $script:mockPSResourceRepositoryInstance.Set() $script:mockMethodModifyCallCount | Should -Be 1
We can add a blank line before this one to get som air between the two lines.
Code quote:
$script:mockMethodModifyCallCount | Should -Be 1
tests/Unit/Classes/PSResourceRepository.Tests.ps1
line 482 at r45 (raw file):
It 'Should return the correct result when the Repository is present and all params are passed' {
We can remove this blank line
tests/Unit/Classes/PSResourceRepository.Tests.ps1
line 494 at r45 (raw file):
} $currentState = $script:mockPSResourceRepositoryInstance.GetCurrentState(@{
Missing the evaluation of the properties Credential
, Proxy
, ProxyCredential
, and Default
so they return the expected value ($null
?).
Code quote:
$currentState = $script:mockPSResourceRepositoryInstance.GetCurrentState(@{
tests/Unit/Classes/PSResourceRepository.Tests.ps1
line 494 at r45 (raw file):
} $currentState = $script:mockPSResourceRepositoryInstance.GetCurrentState(@{
Missing the evaluation of the properties Credential
, Proxy
, ProxyCredential
, and Default
so they return the expected value when they are passed a value through configuration. Note, there is not possible to get Credentials to return, so okay if they always return $null or like we did here https://github.com/dsccommunity/SqlServerDsc/blob/d5692e24b3b6303556729639cbb0b2a6763198af/source/Classes/020.SqlDatabasePermission.ps1#L203-L211.
Code quote:
$currentState = $script:mockPSResourceRepositoryInstance.GetCurrentState(@{
tests/Unit/Classes/PSResourceRepository.Tests.ps1
line 495 at r45 (raw file):
$currentState = $script:mockPSResourceRepositoryInstance.GetCurrentState(@{ Name = 'FakePSGallery'
Wrong identation here, should be moved back one step. Suggest changing it to
Suggestion:
$currentState = $script:mockPSResourceRepositoryInstance.GetCurrentState(
@{
Name = 'FakePSGallery'
}
)
tests/Unit/Classes/PSResourceRepository.Tests.ps1
line 757 at r45 (raw file):
} ) } | Should -Throw -ExpectedMessage 'SourceLocation is a required parameter to register a repository.'
Instead of hardcoding the localization strings we can re-use the same as the code does. This should work I think.
Suggestion:
$script:mockPSResourceRepositoryInstance.localizedData.SourceLocationRequiredForRegistration
tests/Unit/Classes/PSResourceRepository.Tests.ps1
line 781 at r45 (raw file):
{ $script:mockPSResourceRepositoryInstance.Modify(@{ Ensure = 'Absent'
Same as previous a comment, suggest moving the start of hashtable to next line.
Code quote:
$script:mockPSResourceRepositoryInstance.Modify(@{
Ensure = 'Absent'
}
)
tests/Unit/Classes/PSResourceRepository.Tests.ps1
line 807 at r45 (raw file):
InModuleScope -ScriptBlock { { $script:mockPSResourceRepositoryInstance.Modify(@{
Same as previous a comment, suggest moving the start of hashtable to next line.
Code quote:
$script:mockPSResourceRepositoryInstance.Modify(@{
SourceLocation = 'https://www.fakepsgallery.com/api/v2'
}
)
tests/Unit/Classes/PSResourceRepository.Tests.ps1
line 834 at r45 (raw file):
$credential = New-Object -TypeName System.Management.Automation.PSCredential -ArgumentList 'USER', $securePassword $script:mockPSResourceRepositoryInstance.ProxyCredental = $credential $script:mockPSResourceRepositoryInstance.AssertProperties() | Should -Throw -ExpectedMessage 'Proxy Credential passed without Proxy Uri.'
We can use the localized string here too, see a previous comment.
Code quote:
-ExpectedMessage 'Proxy Credential passed without Proxy Uri.'
tests/Unit/Classes/PSResourceRepository.Tests.ps1
line 846 at r45 (raw file):
$script:mockPSResourceRepositoryInstance.Name = 'PSGallery' $script:mockPSResourceRepositoryInstance.Default = $false $script:mockPSResourceRepositoryInstance.AssertProperties() | Should -Throw -ExpectedMessage 'The parameter Default must be set to True for a repository named PSGallery.'
We can use the localized string here too, see a previous comment.
Code quote:
ExpectedMessage 'The parameter Default must be set to True for a repository named PSGallery.'
tests/Unit/Classes/PSResourceRepository.Tests.ps1
line 867 at r45 (raw file):
$script:mockPSResourceRepositoryInstance.Name = 'NotTheDefaultPSGallery' $script:mockPSResourceRepositoryInstance.Default = $true $script:mockPSResourceRepositoryInstance.AssertProperties() | Should -Throw -ExpectedMessage 'The parameter Default may only be used with repositories named PSGallery'
We can use the localized string here too, see a previous comment.
Code quote:
-ExpectedMessage 'The parameter Default may only be used with repositories named PSGallery'
tests/Unit/Classes/PSResourceRepository.Tests.ps1
line 878 at r45 (raw file):
$script:mockPSResourceRepositoryInstance.Default = $true $script:mockPSResourceRepositoryInstance.SourceLocation = 'https://notaurl.com/' $script:mockPSResourceRepositoryInstance.AssertProperties() | Should -Throw
Can we use the ErrorId to evaluate if we get Should -Throw -ErrorId 'DRC0010' in the error message? Alternative, if the error id is not correctly passed, we look for it in the string like
Should -Throw -ExpectedMessage 'DRC0010'`. Probably the later will work...
Either way, then we don't just look for any error.
Throughout on the ones below too.
Suggestion:
Should -Throw -ExpectedMessage 'DRC0010'
tests/Unit/Classes/PSResourceRepository.Tests.ps1
line 933 at r45 (raw file):
$script:mockPSResourceRepositoryInstance.Default = $true $script:mockPSResourceRepositoryInstance.PackageManagementProvider = 'Package' $script:mockPSResourceRepositoryInstance.AssertProperties() | Should -Throw -ExpectedMessage 'i just want to see the message'
STrange that this didn't throw as it should not be what the method should throw?
Code quote:
Should -Throw -ExpectedMessage 'i just want to see the message'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 26 of 29 files reviewed, 27 unresolved discussions (waiting on @johlju and @PlagueHO)
source/Classes/020.PSResourceRepository.ps1
line 181 at r45 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
We can remove the blank line her
Done.
source/Classes/020.PSResourceRepository.ps1
line 241 at r45 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
We can now remove this function.
Done.
source/en-US/PSResourceRepository.strings.psd1
line 18 at r45 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
When the function is removed as per previous comment, I think we can remove this localization string also.
Done.
tests/Unit/Classes/PSResourceRepository.Tests.ps1
line 65 at r45 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
We can remove this blank line
Done.
tests/Unit/Classes/PSResourceRepository.Tests.ps1
line 69 at r45 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
We can remove this blank line
Done.
tests/Unit/Classes/PSResourceRepository.Tests.ps1
line 85 at r45 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
We should indent this one step to show it is part of the pipeline that is started on the previous line. Throughout the test code on similiar code.
Done.
tests/Unit/Classes/PSResourceRepository.Tests.ps1
line 96 at r45 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Missing the evaluation of the properties
Credential
,Proxy
,ProxyCredential
, andDefault
so they return the expected value ($null
?).
Done.
tests/Unit/Classes/PSResourceRepository.Tests.ps1
line 138 at r45 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Missing the evaluation of the properties
Credential
,Proxy
,ProxyCredential
, andDefault
so they return the expected value ($null
?).
Done.
tests/Unit/Classes/PSResourceRepository.Tests.ps1
line 168 at r45 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Missing the evaluation of the properties
Credential
,Proxy
,ProxyCredential
, andDefault
so they return the expected value ($null
?).
Done.
tests/Unit/Classes/PSResourceRepository.Tests.ps1
line 184 at r45 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
We can remove this blank line
Done.
tests/Unit/Classes/PSResourceRepository.Tests.ps1
line 188 at r45 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
We don't need to provide this when the configuration should be absent.
Done.
tests/Unit/Classes/PSResourceRepository.Tests.ps1
line 235 at r45 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
These three should probably return $null since we are mocking the repository to be absent?
The repository is currently Present so Get() is going to populate those keys.
tests/Unit/Classes/PSResourceRepository.Tests.ps1
line 302 at r45 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
We don't need to provide this when the configuration should be absent.
Done.
tests/Unit/Classes/PSResourceRepository.Tests.ps1
line 402 at r45 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
We can add a blank line before this one to get som air between the two lines.
Done.
tests/Unit/Classes/PSResourceRepository.Tests.ps1
line 482 at r45 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
We can remove this blank line
Done.
tests/Unit/Classes/PSResourceRepository.Tests.ps1
line 494 at r45 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Missing the evaluation of the properties
Credential
,Proxy
,ProxyCredential
, andDefault
so they return the expected value ($null
?).
Done.
tests/Unit/Classes/PSResourceRepository.Tests.ps1
line 494 at r45 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Missing the evaluation of the properties
Credential
,Proxy
,ProxyCredential
, andDefault
so they return the expected value when they are passed a value through configuration. Note, there is not possible to get Credentials to return, so okay if they always return $null or like we did here https://github.com/dsccommunity/SqlServerDsc/blob/d5692e24b3b6303556729639cbb0b2a6763198af/source/Classes/020.SqlDatabasePermission.ps1#L203-L211.
I'm mostly skipping testing these parameters because I'm not sure exactly how Get-PSRespository
presents them and I can't set up a complicated config like that. IMO, PSRepositories with ProxyCredential
, Proxy
or Credentials
are rare configurations and I'd love to leave them as is in the resource, potentially with an open issue that configurations using those parameters are untested.
tests/Unit/Classes/PSResourceRepository.Tests.ps1
line 495 at r45 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Wrong identation here, should be moved back one step. Suggest changing it to
Done.
tests/Unit/Classes/PSResourceRepository.Tests.ps1
line 757 at r45 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Instead of hardcoding the localization strings we can re-use the same as the code does. This should work I think.
Done.
tests/Unit/Classes/PSResourceRepository.Tests.ps1
line 781 at r45 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Same as previous a comment, suggest moving the start of hashtable to next line.
Done.
tests/Unit/Classes/PSResourceRepository.Tests.ps1
line 807 at r45 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Same as previous a comment, suggest moving the start of hashtable to next line.
Done.
tests/Unit/Classes/PSResourceRepository.Tests.ps1
line 834 at r45 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
We can use the localized string here too, see a previous comment.
Done.
tests/Unit/Classes/PSResourceRepository.Tests.ps1
line 846 at r45 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
We can use the localized string here too, see a previous comment.
Done.
tests/Unit/Classes/PSResourceRepository.Tests.ps1
line 867 at r45 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
We can use the localized string here too, see a previous comment.
Done.
tests/Unit/Classes/PSResourceRepository.Tests.ps1
line 878 at r45 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Can we use the ErrorId to evaluate if we get
Should -Throw -ErrorId 'DRC0010' in the error message? Alternative, if the error id is not correctly passed, we look for it in the string like
Should -Throw -ExpectedMessage 'DRC0010'`. Probably the later will work...
Either way, then we don't just look for any error.Throughout on the ones below too.
Done.
tests/Unit/Classes/PSResourceRepository.Tests.ps1
line 933 at r45 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
STrange that this didn't throw as it should not be what the method should throw?
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I just hit everything!
Reviewable status: 26 of 29 files reviewed, 27 unresolved discussions (waiting on @johlju and @PlagueHO)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one more comment.
Reviewed 2 of 3 files at r46, 1 of 1 files at r47, 1 of 1 files at r48, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @nickgw and @PlagueHO)
tests/Unit/Classes/PSResourceRepository.Tests.ps1
line 235 at r45 (raw file):
Previously, nickgw (Nick G) wrote…
The repository is currently Present so Get() is going to populate those keys.
But the It-block description says the current state is absent (repository does not exist in current state), in that case the GetCurrentState would not return the properties. 🤔 Maybe I'm missing something.
tests/Unit/Classes/PSResourceRepository.Tests.ps1
line 494 at r45 (raw file):
Previously, nickgw (Nick G) wrote…
I'm mostly skipping testing these parameters because I'm not sure exactly how
Get-PSRespository
presents them and I can't set up a complicated config like that. IMO, PSRepositories withProxyCredential
,Proxy
orCredentials
are rare configurations and I'd love to leave them as is in the resource, potentially with an open issue that configurations using those parameters are untested.
Let us make other contributors create an issue for it if there is an issue. Looks good as-is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 28 of 29 files reviewed, 2 unresolved discussions (waiting on @johlju and @PlagueHO)
tests/Unit/Classes/PSResourceRepository.Tests.ps1
line 235 at r45 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
But the It-block description says the current state is absent (repository does not exist in current state), in that case the GetCurrentState would not return the properties. 🤔 Maybe I'm missing something.
Ah duh. I should have looked a little more closely. My bad mock of GetCurrentState()
threw me off. This should be fixed now.
Code quote:
hould return the correct result when the Repository is absent but should be present
There was a problem hiding this 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 r49, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @PlagueHO)
tests/Unit/Classes/PSResourceRepository.Tests.ps1
line 235 at r45 (raw file):
Previously, nickgw (Nick G) wrote…
Ah duh. I should have looked a little more closely. My bad mock of
GetCurrentState()
threw me off. This should be fixed now.
No worries 🙂
I will keep an eye on the build jobs and restart them if any fail due to unrelated issues to this PR. 🙂 Then I merge. |
There was a problem hiding this 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 r50, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @PlagueHO)
@nickgw awesome work on this! It is now merged. 🙂 |
@johlju thanks so much! enjoy your vacation!!! |
Pull Request (PR) description
New resources to manage PowerShell repositories and PowerShell modules:
PSResourceRepository
This Pull Request (PR) fixes the following issues
Task list
file CHANGELOG.md. Entry should say what was changed and how that
affects users (if applicable), and reference the issue being resolved
(if applicable).
and comment-based help.
This change is