-
Notifications
You must be signed in to change notification settings - Fork 7
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 ready for refactor #39
Conversation
FYI. I'm gonna switch the default branch back to main otherwise the pipeline wont work. Having another branch is gonna be a pain and just add even more maintenance. |
I was going to ask why build was not triggering. Where is best to discuss this module as a whole? With the changes in #37, it would be good to try and unpick that into smaller PR's. |
Best would to discuss it in the PR that suggest changes, as those interested in the module can have an opinion. 🤔 But the PR has been worked on for a long time without anyone in community that is using the module has raised any concerns or suggestions, so a few weeks back I assumed it is a general good change and would have just merged it. 🤔 |
It's a pretty large change and most of the tests are not using the dsccommunity conventions or tasks. I'm not sure how much the module is being used either, I can't find it in the PSGallery. Any reason you can see for why azure pipelines are not running? |
/azp run |
Azure Pipelines failed to run 1 pipeline(s). |
It says
|
Just seen it. |
Maybe split it up with just one DSC resource with all necessary helper code to make it run so that can be reviewed first. 🤔
There are no releases or tags in the repo so my guess it has never been used since until recently there was not a pipeline added, and no changes to source after it was added that would have triggered a preview build. |
Suggest removing the Linux, macOS and PS Core unit tests, let us assume this will be used in Windows only until another PR introduces something else? 🤔 |
That's my initial thought, use it to get CI to pass then it will be much easier for future PR's.
That makes sense. |
Waiting on dsccommunity/DscResource.Common#134 for HQRM to pass. |
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 71 files reviewed, 1 unresolved discussion (waiting on @dan-hughes)
build.yaml
line 115 at r4 (raw file):
ExcludeSourceFile: - output ExcludeModuleFile:
We should add - Modules/DscResource.Common
to exclude and modules that is not part of source
in the built moduled
Yes. I've disabled Integration tests for the time being. Just need to fix code coverage merging. |
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
@johlju, this should also be green when it finishes. Next steps, pick a resource out of the other PR and add the minimum files. But not in this PR. |
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 50 of 70 files at r1, 19 of 19 files at r3, 1 of 2 files at r6, 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dan-hughes)
.devcontainer/devcontainer.json
line 4 at r7 (raw file):
// README at: https://github.com/devcontainers/templates/tree/main/src/powershell { "name": "PowerShell",
Looks like tabs in this file.
Can you check the tabs in the file (see comment), then I merge it. |
As in convert it to spaces? Not sure why it did that. |
Exactly, convert to spaces if possible. |
Done. |
Pull Request (PR) description
Update the repository files to prepare for the larger changes coming.
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