-
Notifications
You must be signed in to change notification settings - Fork 8
Fix DSC 3.0 for resources #45
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
base: main
Are you sure you want to change the base?
Fix DSC 3.0 for resources #45
Conversation
WalkthroughTwo validation constraints were relaxed: the AzDevOpsProject class's SourceControlType property no longer enforces predefined values ('Git', 'Tfvc') through ValidateSet, and the Test-AzDevOpsPat function removes the length check requirement for PAT validation. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@source/Modules/AzureDevOpsDsc.Common/Resources/Functions/Private/Test-AzDevOpsPat.ps1`:
- Line 39: Update the comment-based help for Test-AzDevOpsPat (in
Test-AzDevOpsPat.ps1) to state that the function only validates that the PAT is
non-empty/non-whitespace (not its length or format), and then update the test
definitions in CommonTestCases.psm1 (the PAT cases around the 222-233 block) to
treat any non-empty string as valid (remove the 52-character exact-length
restriction) so tests match the implementation; alternatively, if DSC 3.0
requires format validation, restore the length/format check inside
Test-AzDevOpsPat instead and keep the tests as-is—pick one of these two paths
and make the help, implementation (Test-AzDevOpsPat) and test data
(CommonTestCases.psm1) consistent.
🧹 Nitpick comments (3)
source/Modules/AzureDevOpsDsc.Common/Resources/Functions/Private/Test-AzDevOpsPat.ps1 (1)
1-22: Consider completing comment-based help sections.The comment-based help is missing
.DESCRIPTION(should be 40+ chars per guidelines),.INPUTS, and.OUTPUTSsections. Since this is an existing file with a minor change, this can be addressed in a future PR.source/AzureDevOpsDsc.psd1 (2)
5-5: Property naming inconsistency.
moduleVersionuses camelCase while other manifest properties use PascalCase (e.g.,RootModule,CompanyName). Consider usingModuleVersionfor consistency.♻️ Suggested fix
- moduleVersion = '0.3.0' + ModuleVersion = '0.3.0'
46-46: Extra blank line.There are two consecutive blank lines between
NestedModulesandDscResourcesToExport. One blank line would be sufficient for separation.
source/Modules/AzureDevOpsDsc.Common/Resources/Functions/Private/Test-AzDevOpsPat.ps1
Show resolved
Hide resolved
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.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@source/Classes/010.AzDevOpsProject.ps1`:
- Around line 39-41: Remove the commented-out ValidateSet line for the
SourceControlType property (the "[ValidateSet('Git', 'Tfvc')]" comment) and, if
you still need to enforce allowed values, implement runtime validation in the
class's AssertProperties() or NormalizeProperties() method: check that the
SourceControlType property is either "Git" or "Tfvc" and throw/normalize
accordingly. This keeps the property declaration clean and moves schema
enforcement to AssertProperties()/NormalizeProperties() as suggested.
Pull Request (PR) description
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).
This change is