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

BREAKING CHANGE: xWebAdministration renamed to WebAdministrationDsc: Renamed all DSCResources, Examples, Modules and Tests where applicable #601

Merged
merged 74 commits into from
Jun 8, 2022

Conversation

nickgw
Copy link
Contributor

@nickgw nickgw commented May 24, 2022

Pull Request (PR) description

xWebAdministration renamed to WebAdministrationDsc

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 added/updated in README.md.
  • Resource parameter descriptions added/updated in README.md, schema.mof
    and comment-based help.
  • Comment-based help added/updated.
  • Localization strings added/updated in all localization files as appropriate.
  • Examples appropriately added/updated.
  • Unit tests added/updated. See DSC Community Testing Guidelines.
  • Integration tests added/updated (where possible). See DSC Community Testing Guidelines.
  • New/changed code adheres to DSC Community Style Guidelines.

This change is Reviewable

@johlju johlju self-requested a review May 30, 2022 17:01
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.

Reviewed 2 of 144 files at r1, 1 of 1 files at r2, 22 of 42 files at r3, 1 of 1 files at r4.
Reviewable status: all files reviewed (commit messages unreviewed), 6 unresolved discussions (waiting on @nickgw)


tests/TestHelper/CommonTestHelper.psm1 line 238 at r4 (raw file):

#>
function New-SelfSignedCertificateEx {
    [OutputType('[System.Security.Cryptography.X509Certificates.X509Certificate2]')]

We should not need this as we use the command from the module PSPKI. 🤔 See https://github.com/dsccommunity/xWebAdministration/blob/334575ac032f2b04f98bdaed1149f904d5d35899/tests/Unit/xWebAdministration.Common.Tests.ps1#L27

I added it in one of the PR's I pused yesterday.

Code quote:

New-SelfSignedCertificateEx

@johlju
Copy link
Member

johlju commented May 30, 2022

@nickgw it seems this row fails in the integration tests:

https://github.com/dsccommunity/xWebAdministration/blob/df669ce662ff3e03ad1268498086204f34cb8717/tests/Integration/DSC_WebApplication.Integration.Tests.ps1#L102

I'm think, can it be that we run the integration tests to fast. Can we test adding Start-Sleep -Seconds 5 before running the command? If that doesn't work I suggest comment that test, the create an issue to track it and we move on. Let us in that case fix it in another PR. What do you think?

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.

Reviewed 4 of 4 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @nickgw)


README.md line 8 at r5 (raw file):

![Azure DevOps coverage (branch)](https://img.shields.io/azure-devops/coverage/dsccommunity/WebAdministrationDsc/7/main)
[![Azure DevOps tests](https://img.shields.io/azure-devops/tests/dsccommunity/WebAdministrationDsc/7/main)](https://dsccommunity.visualstudio.com/WebAdministrationDsc/_test/analytics?definitionId=7&contextType=build)
[![codecov](https://codecov.io/gh/dsccommunity/xWebAdministration/branch/main/graph/badge.svg)](https://codecov.io/gh/dsccommunity/xWebAdministration)

Should be:

Suggestion:

WebAdministrationDsc

@johlju
Copy link
Member

johlju commented May 30, 2022

It is like the Get-TargetResource function throws an error or returns some unexpected value that Get-DscConfiguration does not like. 🤔

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: 160 of 161 files reviewed, 2 unresolved discussions (waiting on @johlju and @nickgw)


tests/Integration/DSC_WebApplication.Integration.Tests.ps1 line 74 at r7 (raw file):

        It 'Should compile without throwing' {
            {
                Invoke-Expression -Command "$($script:dscResourceName)_Present -ConfigurationData `$DSCConfig -OutputPath `$TestDrive"

This should be $configData now?

Code quote:

$DSCConfig

@johlju johlju self-requested a review May 30, 2022 19:02
@johlju
Copy link
Member

johlju commented May 30, 2022

Now it fails with the previous error again. Looking at the tests thay run for xWeb I see the do run in a different order now... It might be something a previous test added that made the test, by luck, work previously. 🤔

@johlju
Copy link
Member

johlju commented May 30, 2022

I think the integration test need to be refactored, like you tried too. But we can do that in another PR. I happy to comment out the test and make an issue for it to be fixed, by someone, in another PR.

@nickgw
Copy link
Contributor Author

nickgw commented May 30, 2022

I think the integration test need to be refactored, like you tried too. But we can do that in another PR. I happy to comment out the test and make an issue for it to be fixed, by someone, in another PR.

Created this for the failing integration test: #606

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 2 of 2 files at r10, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @nickgw)

@johlju johlju added the on hold The issue or pull request has been put on hold by a maintainer. label Jun 1, 2022
@johlju
Copy link
Member

johlju commented Jun 1, 2022

Awesome work @nickgw! This PR is ready, I will rename this repo as soon as I have a chance.

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 13 of 13 files at r12, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @nickgw)

@johlju
Copy link
Member

johlju commented Jun 3, 2022

Waiting for the API key to be update on the pipeline so we can merge this.

@johlju johlju removed the on hold The issue or pull request has been put on hold by a maintainer. label Jun 8, 2022
@johlju johlju merged commit e479b30 into dsccommunity:main Jun 8, 2022
@nickgw nickgw deleted the nickgw_renamewebadministrationdsc branch June 8, 2022 13:03
Clay10J pushed a commit to Clay10J/WebAdministrationDsc that referenced this pull request Dec 5, 2023
…Renamed all DSCResources, Examples, Modules and Tests where applicable (dsccommunity#601)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants