-
Notifications
You must be signed in to change notification settings - Fork 67
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
Update repo files + change .Common to a buildable module #219
base: main
Are you sure you want to change the base?
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.
Added some comments for discussion. Let me know what you think. 🙂
Reviewed 37 of 37 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dan-hughes)
source/Public/Get-VMHyperV.ps1
line 8 at r1 (raw file):
Name of the Hyper-V virtual machine to return #> function Get-VMHyperV
If a user has another command with this name exported from another module this is gonna be a conflict. To avoid that we should probably prefix the public commands, e.g. Get-HyperVDscVM
.
Same for other commands.
Code quote:
Get-VMHyperV
source/Public/ConvertFrom-TimeSpan.ps1
line 11 at r1 (raw file):
Convert timespan into the total number of seconds, minutes, hours or days. #> function ConvertFrom-TimeSpan
Maybe this should be moved to DscResource.Common? But can be another PR.
If a user has another command with this name exported from another module this is gonna be a conflict. To avoid that we should probably prefix the public commands, e.g. ConvertFrom-HyperVDscTimeSpan
.
By moving it to DscResource.Common and because the module is imported inside the HyperVDsc module scope we avoid conflicts even if not renamed... I think. 🤔
Same for ConvertTo-TimeSpan
Code quote:
ConvertFrom-TimeSpan
I'm happy to do so, this is an obvious downside to the functions not being nested inside a module inside the Dsc module. Is there a way of having a nested Sampler module that maybe is best of both worlds? e.g. still have HyperVDsc.Common but it has it's own build.yaml inside this repo? |
Example of adding the module name as a suffix. |
9e5220d
to
a755107
Compare
Pull Request (PR) description
Update the module files to the latest versions. Move functions from the HyperVDsc.Common module to separate Public/Private functions.
Fixed typo in one integration test that referenced old module name.
This Pull Request (PR) fixes the following issues
Task list
Entry should say what was changed and how that affects users (if applicable), and
reference the issue being resolved (if applicable).
This change is