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 Public Function Find-Certificate #101

Merged
merged 16 commits into from
Apr 6, 2023

Conversation

hollanjs
Copy link
Contributor

@hollanjs hollanjs commented Feb 15, 2023

Pull Request (PR) description

Moved Find-Certificate from a utility function in CertificateDsc.Common.psm1 to a public function in DscResource.Common.
Related to CertificateDsc Issue #272

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).
  • Documentation added/updated in README.md.
  • Comment-based help added/updated for all new/changed functions.
  • 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

@codecov
Copy link

codecov bot commented Feb 15, 2023

Codecov Report

Merging #101 (ed4d7d9) into main (c5ef7be) will increase coverage by 0%.
The diff coverage is 100%.

❗ Current head ed4d7d9 differs from pull request most recent head 3a0111b. Consider uploading reports for the commit 3a0111b to get more accurate results

Impacted file tree graph

@@         Coverage Diff         @@
##           main   #101   +/-   ##
===================================
  Coverage    91%    92%           
===================================
  Files        34     35    +1     
  Lines       599    628   +29     
===================================
+ Hits        550    579   +29     
  Misses       49     49           
Impacted Files Coverage Δ
source/Public/Find-Certificate.ps1 100% <100%> (ø)

@hollanjs hollanjs marked this pull request as ready for review February 15, 2023 20:01
@johlju johlju self-requested a review March 26, 2023 09:57
@johlju
Copy link
Member

johlju commented Mar 26, 2023

I forgot about this. I have been working a lot lately so I haven't been able do much community work. Will try to look at this soon.

@johlju johlju added the needs review The pull request needs a code review. label Apr 1, 2023
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 3 of 5 files at r1, 1 of 1 files at r2, 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @hollanjs)


tests/Unit/Public/Find-Certificate.Tests.ps1 line 154 at r4 (raw file):

        It 'Should call expected mocks' {
            Assert-MockCalled -CommandName Test-Path -Exactly -Times 1 -Scope "context"

We can replace all these aliases with Should -Invoke -CommandName since this repo is using Pester v5. Throughout the tests.

Suggestion:

Should -Invoke -CommandName

@johlju
Copy link
Member

johlju commented Apr 1, 2023

@hollanjs sorry for tasking so long wit this one. Just a tiny comment.

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 all commit messages.
Reviewable status: 4 of 5 files reviewed, all discussions resolved (waiting on @hollanjs)


tests/Unit/Public/Find-Certificate.Tests.ps1 line 154 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

We can replace all these aliases with Should -Invoke -CommandName since this repo is using Pester v5. Throughout the tests.

Done. I sent in a commit to fix this so we can merge this.

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 1 of 1 files at r5.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @hollanjs)

@johlju johlju added ready for merge The pull request was approved by the community and is ready to be merged by a maintainer. and removed needs review The pull request needs a code review. labels Apr 6, 2023
@johlju johlju merged commit fd3c5f6 into dsccommunity:main Apr 6, 2023
8 checks passed
@johlju johlju removed the ready for merge The pull request was approved by the community and is ready to be merged by a maintainer. label Apr 6, 2023
johlju pushed a commit that referenced this pull request Apr 6, 2023
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.

Add Find-Certificate from CertificateDsc => DscResource.Common Public Functions
2 participants