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

xWebSite: Choose cert with greatest validity #579

Conversation

ThomasHughesIV
Copy link

@ThomasHughesIV ThomasHughesIV commented Jul 2, 2020

Pull Request (PR) description

This Pull Request (PR) fixes the following issues

Task list

  • [x ] 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.
  • [x ] New/changed code adheres to DSC Community Style Guidelines.

This change is Reviewable

@ThomasHughesIV
Copy link
Author

Wow, is pester updated or something? I cannot see how this failed so badly in build when changing one line of code with Where-Object statements

@johlju
Copy link
Member

johlju commented Jul 5, 2020

Yes, Pester 5 was released so we need to pin Pester to 4.10.1 here

https://github.com/dsccommunity/xWebAdministration/blob/c7c81dcdca4102b5e06f0ffed16b35d1b7f6c07b/RequiredModules.psd1#L11

@johlju johlju changed the title X website choose cert with greatest validity xWebSite: Choose cert with greatest validity Jul 5, 2020
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 2 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ThomasHughesIV)

a discussion (no related file):
Can you please add a unit test (or change an existing) so that the correct certificate is chosen?

https://github.com/dsccommunity/xWebAdministration/blob/c7c81dcdca4102b5e06f0ffed16b35d1b7f6c07b/tests/Unit/MSFT_xWebsite.Tests.ps1#L2273


@johlju johlju added the waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. label Jul 5, 2020
@ThomasHughesIV
Copy link
Author

Reviewed 2 of 2 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ThomasHughesIV)

a discussion (no related file):
Can you please add a unit test (or change an existing) so that the correct certificate is chosen?

https://github.com/dsccommunity/xWebAdministration/blob/c7c81dcdca4102b5e06f0ffed16b35d1b7f6c07b/tests/Unit/MSFT_xWebsite.Tests.ps1#L2273

Yes, I can do that. I think. I'm still new to all of this.

@johlju
Copy link
Member

johlju commented Jul 8, 2020

Ping me if I can help in some way.

@ThomasHughesIV
Copy link
Author

Ping me if I can help in some way.

I appreciate that, thank you. This might take some time, as I'm trying to determine how certs are generated by pester (and learn pester at the same time)

@johlju
Copy link
Member

johlju commented Jul 8, 2020

In a unit test you mock cmdlets/functions with Pester. So you mock what a cmdlet should return. So if you have a function let say Get-Certificate you mock it to return the certificate (the necessary object with properties).

Example below. Not sure this is the correct way to mock the certificate in this case but gives you an idea.

Mock -CommandName Get-Certificate -MockWith {
    return [PSCustomObject] @{
        SubjectName = ‘cert1’
        Property2 = ‘value2’
    }
}

@johlju johlju changed the base branch from master to main January 7, 2021 19:17
@johlju
Copy link
Member

johlju commented Jun 8, 2022

We have renamed the resource, removing 'x', so please rebase this PR.

@johlju johlju closed this in #633 May 22, 2024
@johlju johlju removed the waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. label May 22, 2024
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.

None yet

2 participants