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

PfxImport: Add empty string password to .Import method #259

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

cmielke
Copy link

@cmielke cmielke commented Aug 10, 2021

Pull Request (PR) description

Add an empty string to the X509Certificate2.Import to prevent the MethodCountCouldNotFindBest exception when using a null password to import the PFX certificate.

This Pull Request (PR) fixes the following issues

- Fixes #258 

Task list

  • Added an entry under the Unreleased section of the CHANGELOG.md.
    Entry should say what was changed, and how that affects users (if applicable).
  • Resource documentation added/updated in the resource folder.
  • Resource parameter descriptions added/updated in 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

@codecov
Copy link

codecov bot commented Aug 10, 2021

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Project coverage is 84%. Comparing base (e77b3da) to head (73826cd).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
...s/CertificateDsc.Common/CertificateDsc.Common.psm1 0% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@         Coverage Diff         @@
##           main   #259   +/-   ##
===================================
  Coverage    84%    84%           
===================================
  Files         7      7           
  Lines       933    933           
===================================
  Hits        790    790           
  Misses      143    143           
Files with missing lines Coverage Δ
...s/CertificateDsc.Common/CertificateDsc.Common.psm1 81% <0%> (ø)

@PlagueHO PlagueHO 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 May 31, 2022
@bdrans
Copy link

bdrans commented Dec 13, 2022

Does anyone know what needs to be changed with the PR to get it merged? I'm running into this issue (#258) so it would be nice go get it merged

Copy link

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

Copy link

@hebbea hebbea left a comment

Choose a reason for hiding this comment

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

Looks and works ok

@PlagueHO PlagueHO self-requested a review July 3, 2023 10:00
@PlagueHO
Copy link
Member

PlagueHO commented Jul 3, 2023

Hi @cmielke - any chance you could rebase this against main? There have been a few fixes/changes to the pipeline. This should fix the build process so that all the tests pass. Thank you!

@cmielke
Copy link
Author

cmielke commented Jul 3, 2023

I have rebased against main. Hopefully that will work.

@PlagueHO PlagueHO added needs review The pull request needs a code review. and removed waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. labels Oct 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs review The pull request needs a code review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants