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

Remove Write-Host from Get-EnterprisePKIHealthStatus #4

Closed
tdabasinskas opened this issue Sep 13, 2016 · 9 comments
Closed

Remove Write-Host from Get-EnterprisePKIHealthStatus #4

tdabasinskas opened this issue Sep 13, 2016 · 9 comments
Labels
bug Bug. An issue exist in our code.

Comments

@tdabasinskas
Copy link

Hi,

As per the best practices, cmdlets should not be using Write-Host function to display any output. Anyhow, it seems that Get-EnterprisePKIHealthStatus cmdlet does not comply with that, always outputting the CA name due to the following line:

Write-Host ("=" * 20) $CA.DisplayName ("=" * 20)

Is it possible to remove this line, convert it to Write-Verbose or introduce an additional switch parameter which would allow to suppress it?

Thank you.

@Crypt32
Copy link
Collaborator

Crypt32 commented Sep 13, 2016

Looks like it is an artifact from my early POCO version. Although, there is a such recommendation to not use Write-Host, I'm still using it in several cmdlets because there is no better way to respond to user.

However, this issue is not the case and I fixed it in 3aab19f. I will work further to remove all ambigous Write-Host occurences. For consistency puproses, please open the "Write-Host" and "Write-Warning" issues against the following commands. so I'll be able to track them:

Approve-CertificateRequest
Get-RequestRow
Remove-CAKRACertificate
Revoke-Certificate
Remove-CertificateEnrollmentPolicyService
Remove-CertificateEnrollmentService
Remove-DatabaseRow
Deny-CertificateRequest
Add-CertificateEnrollmentService
Add-CertificateEnrollmentPolicyService
Add-CertificateTemplateAcl

You are not required to close this issue, because I'll close it when publishing next module release.

@Crypt32 Crypt32 added the bug Bug. An issue exist in our code. label Sep 13, 2016
@tdabasinskas
Copy link
Author

Thanks, @Crypt32! Is there actually a reason you would suggest removing Write-Warning as well? As far as I know, this one is OK to use (which is not the case with Write-Host).

@Crypt32
Copy link
Collaborator

Crypt32 commented Sep 13, 2016

Is there actually a reason you would suggest removing Write-Warning as well?

yep. The module was created long ago before there guidelines were created (2009, or 2010). Originally, PowerShell community suggested to use Write-Warning instead of Write-Error to indicate error messages. There are several parts that are unchanged from that time, I think that it is better to throw errors when appropriate instead of Write-Warning. The functions I mentioned use this outdated behavior to indicate various sorts of errors or status messages,

@Crypt32
Copy link
Collaborator

Crypt32 commented Sep 13, 2016

I mean that Write-Warning will remain where it is appropriate (when the cmdlet notifies about further required steps), but definitely must be changed where it is used to indicate error state. These occurences should be changed with throw statement,

@tdabasinskas
Copy link
Author

Thanks for the clarification. I am going to open a separate issue to have this fixed in the functions you listed.

@Crypt32
Copy link
Collaborator

Crypt32 commented Sep 13, 2016

Yes, please do. I checked listed functions and it appears that Write-Warning there is not appropriate there and throw statement should be used instead.

@Crypt32
Copy link
Collaborator

Crypt32 commented Feb 7, 2018

The work is in progress in this branch: https://github.com/Crypt32/PSPKI/tree/feature-WriteHostCompliance

@FLeven
Copy link

FLeven commented Feb 10, 2018

I am not sure if the back and force, between Write-X (#4, #5 ) is an efficient approach. I can only add, that in a Desired State Configuration / Alternatives world (where we stop to use code in production environemnents, in an interactive fashion and output/report errors to different streams), I still would like to work with your Module(s) instead of what is available as of today. For example the carbon module managed to to be one solution for both worlds and managed to be usefull in designing an environment line by line or in pushing out the result as a DSC configuration in the end.

@Crypt32
Copy link
Collaborator

Crypt32 commented Feb 11, 2018

where we stop to use code in production environemnents

some ideal world. In real world there still are operational tasks which can't be solved by DSC. Therefore, some interactiveness is still necessary.

@Crypt32 Crypt32 added the fixed-vNext The item is fixed in development code. Will be available in next release. label Apr 3, 2018
@Crypt32 Crypt32 removed the fixed-vNext The item is fixed in development code. Will be available in next release. label May 1, 2018
@Crypt32 Crypt32 closed this as completed May 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug. An issue exist in our code.
Projects
None yet
Development

No branches or pull requests

3 participants