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

Added continuous delivery with a new CI pipeline #549

Merged
merged 24 commits into from
Dec 29, 2019
Merged

Added continuous delivery with a new CI pipeline #549

merged 24 commits into from
Dec 29, 2019

Conversation

johlju
Copy link
Member

@johlju johlju commented Dec 28, 2019

Pull Request (PR) description

  • Added continuous delivery with a new CI pipeline

This Pull Request (PR) fixes the following issues

Task list

  • Added an entry to the change log under the Unreleased section of the 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 Resource Testing Guidelines.
  • Integration tests added/updated (where possible). See DSC Resource Testing Guidelines.
  • New/changed code adheres to DSC Resource Style Guidelines and Best Practices.

This change is Reviewable

@johlju johlju added the needs review The pull request needs a code review. label Dec 28, 2019
@PlagueHO PlagueHO self-requested a review December 28, 2019 19:17
Copy link
Member

@PlagueHO PlagueHO left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 157 of 157 files at r1.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @johlju)


azure-pipelines.yml, line 102 at r1 (raw file):

      - job: Test_Integration
        pool:
          vmImage: 'vs2017-win2016'

Why use a different image for unit vs integration tests?


Resolve-Dependency.psd1, line 2 at r1 (raw file):

@{ # Defaults Parameter value to be loaded by the Resolve-Dependency command (unless set in Bound Parameters)
    #PSDependTarget  = './output/modules'

Maybe remove commented out lines?


source/xWebAdministration.psd1, line 82 at r1 (raw file):

    } # End of PrivateData hashtable
}

Can we remove all this whitespace?


source/en-US/about_xWebAdministration.help.txt, line 5 at r1 (raw file):


SHORT DESCRIPTION
    DSC resources for web servers and components around those.

maybe -> DSC resources for configuring web servers and related components.


source/en-US/about_xWebAdministration.help.txt, line 8 at r1 (raw file):


LONG DESCRIPTION
    This module contains DSC resources for deployment and configuration of web servers and components around those.

maybe -> This module contains DSC resources for deploying and configuring IIS web servers and related components.


source/Modules/xWebAdministration.Common/xWebAdministration.Common.psd1, line 15 at r1 (raw file):

    # Company or vendor of this module
    CompanyName       = 'Dsc Community'

Needs upper case DSC


tests/TestHelper/CommonTestHelper.psm1, line 36 at r1 (raw file):

    $newSelfSignedCertZipPath = Join-Path -Path $OutputPath -ChildPath $newSelfSignedCertZip
    $newSelfSignedCertScriptPath = Join-Path -Path $OutputPath -ChildPath 'New-SelfSignedCertificateEx.ps1'
    if (-not (Test-Path -Path $newSelfSignedCertScriptPath))

Add blank line before


tests/TestHelper/CommonTestHelper.psm1, line 42 at r1 (raw file):

            Remove-Item -Path $newSelfSignedCertZipPath -Force
        }
        Invoke-WebRequest -Uri $newSelfSignedCertURL -OutFile $newSelfSignedCertZipPath

Add blank line before.


tests/Integration/MSFT_XIISLogging.Integration.Tests.ps1, line 21 at r1 (raw file):

Import-Module -Name (Join-Path -Path $PSScriptRoot -ChildPath '..\TestHelper\CommonTestHelper.psm1') -Force

[String] $tempName = "$($script:dscResourceName)_" + (Get-Date).ToString("yyyyMMdd_HHmmss")

FYI, doesn't need the [String] declaration.


tests/Unit/MSFT_xWebAppPoolDefaults.Tests.ps1, line 37 at r1 (raw file):

    InModuleScope $script:dscResourceName {

        Describe "xWebAppPoolDefaults\Get-TargetResource" {

Can be single quotes


tests/Unit/MSFT_xWebAppPoolDefaults.Tests.ps1, line 84 at r1 (raw file):

        }

        Describe "xWebAppPoolDefaults\Test-TargetResource" {

Can be single quotes


tests/Unit/MSFT_xWebAppPoolDefaults.Tests.ps1, line 170 at r1 (raw file):

        }

        Describe "xWebAppPoolDefaults\Set-TargetResource" {

Can be single quotes

Copy link
Member Author

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

Thank you @PlagueHO!

Reviewable status: 132 of 157 files reviewed, 11 unresolved discussions (waiting on @PlagueHO)


azure-pipelines.yml, line 102 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Why use a different image for unit vs integration tests?

Using Windows Server Core felt like it would be optimized for unit tests. Integration tests could not be run on Windows Server Core because it could not install a dependency so I move to Windows Server 2016 Full instead. But I will submit an issue to make sure it runs the integration tests on all available images (Server Core, 2016, and 2019). Submitted issue #550.


Resolve-Dependency.psd1, line 2 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Maybe remove commented out lines?

Done.


source/xWebAdministration.psd1, line 82 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can we remove all this whitespace?

Done.


source/en-US/about_xWebAdministration.help.txt, line 5 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

maybe -> DSC resources for configuring web servers and related components.

Done.


source/en-US/about_xWebAdministration.help.txt, line 8 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

maybe -> This module contains DSC resources for deploying and configuring IIS web servers and related components.

Done. Thank you, updated the repository description as well (on GitHub)! I skipped 'IIS' since I don't want to limit this to just IIS.


source/Modules/xWebAdministration.Common/xWebAdministration.Common.psd1, line 15 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Needs upper case DSC

Done.


tests/TestHelper/CommonTestHelper.psm1, line 36 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Add blank line before

Done.


tests/TestHelper/CommonTestHelper.psm1, line 42 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Add blank line before.

Done.


tests/Unit/MSFT_xWebAppPoolDefaults.Tests.ps1, line 37 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can be single quotes

Done.


tests/Unit/MSFT_xWebAppPoolDefaults.Tests.ps1, line 84 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can be single quotes

Done.


tests/Unit/MSFT_xWebAppPoolDefaults.Tests.ps1, line 170 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can be single quotes

Done.

@johlju
Copy link
Member Author

johlju commented Dec 29, 2019

@PlagueHO Ready for sign off I hope. 😄

Copy link
Member

@PlagueHO PlagueHO left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Great job @johlju ! Another one down!

Reviewed 30 of 30 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@johlju johlju merged commit 42526ca into dev Dec 29, 2019
@johlju johlju deleted the add-new-ci branch December 29, 2019 21:38
@johlju johlju removed the needs review The pull request needs a code review. label Dec 29, 2019
gstorme pushed a commit to gstorme/xWebAdministration that referenced this pull request Feb 14, 2020
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.

xWebAdministration: Converting to new CI pipeline
2 participants