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

Update repo files ready for refactor #39

Merged
merged 10 commits into from
Nov 17, 2024

Conversation

dan-hughes
Copy link
Contributor

@dan-hughes dan-hughes commented Nov 16, 2024

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

  • Added an entry to the change log under the Unreleased section of the
    file CHANGELOG.md. Entry should say what was changed and how that
    affects users (if applicable), and reference the issue being resolved
    (if applicable).
  • Resource documentation updated in the resource's README.md.
  • Resource parameter descriptions updated in schema.mof.
  • Comment-based help updated, including parameter descriptions.
  • Localization strings updated.
  • Examples updated.
  • Unit tests updated. See DSC Community Testing Guidelines.
  • Integration tests updated (where possible). See DSC Community Testing Guidelines.
  • Code changes adheres to DSC Community Style Guidelines.

This change is Reviewable

@johlju
Copy link
Member

johlju commented Nov 17, 2024

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.

@johlju johlju changed the base branch from prerelease to main November 17, 2024 08:49
@dan-hughes
Copy link
Contributor Author

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.

@johlju
Copy link
Member

johlju commented Nov 17, 2024

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. 🤔

@dan-hughes
Copy link
Contributor Author

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?

@johlju
Copy link
Member

johlju commented Nov 17, 2024

/azp run

Copy link

Azure Pipelines failed to run 1 pipeline(s).

@johlju
Copy link
Member

johlju commented Nov 17, 2024

It says

/azure-pipelines.yml (Line: 278, Col: 17): While parsing a block mapping, did not find expected key.
9724

@dan-hughes
Copy link
Contributor Author

Just seen it.

@johlju
Copy link
Member

johlju commented Nov 17, 2024

It's a pretty large change and most of the tests are not using the dsccommunity conventions or tasks.

Maybe split it up with just one DSC resource with all necessary helper code to make it run so that can be reviewed first. 🤔

I'm not sure how much the module is being used either, I can't find it in the PSGallery.

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.

@johlju
Copy link
Member

johlju commented Nov 17, 2024

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? 🤔

@dan-hughes
Copy link
Contributor Author

It's a pretty large change and most of the tests are not using the dsccommunity conventions or tasks.

Maybe split it up with just one DSC resource with all necessary helper code to make it run so that can be reviewed first. 🤔

That's my initial thought, use it to get CI to pass then it will be much easier for future PR's.

I'm not sure how much the module is being used either, I can't find it in the PSGallery.

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.

That makes sense.

@dan-hughes
Copy link
Contributor Author

Waiting on dsccommunity/DscResource.Common#134 for HQRM to pass.

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: 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

@dan-hughes
Copy link
Contributor Author

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.

Copy link

codecov bot commented Nov 17, 2024

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 ☂️

@dan-hughes
Copy link
Contributor Author

dan-hughes commented Nov 17, 2024

@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.

@dan-hughes dan-hughes marked this pull request as ready for review November 17, 2024 16:11
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 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.

@johlju
Copy link
Member

johlju commented Nov 17, 2024

Can you check the tabs in the file (see comment), then I merge it.

@dan-hughes
Copy link
Contributor Author

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.

@johlju
Copy link
Member

johlju commented Nov 17, 2024

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.

@dan-hughes
Copy link
Contributor Author

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.

@johlju johlju merged commit bd9ab0d into dsccommunity:main Nov 17, 2024
10 of 11 checks passed
@dan-hughes dan-hughes deleted the repo-updates branch November 17, 2024 17:44
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.

2 participants