-
-
Notifications
You must be signed in to change notification settings - Fork 67
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
Error handling #13
Comments
Hrmms, you are correct that ErrorAction Stop is required on most of the stuff (something I've learned recrently). Now the question is do we want the script to stop if any error occurs. For example Get-EventLogSize is not critical for the script to continue. If I were running a script for 3 days (like last time I did to rescan 500gb logs) and it would throw an error at the end I would be heavily disappointed. As for Test-Key I believe it was throwing me errors when I used |
For the Get-ADDomain and similar I actually wanted or maybe even did something (can't remember from top of my head) a function that would check for availability of it. I think it's in PSSharedGoods as I use it for PSWinDocumentation and PSAutomator. Or maybe I'm just remembering something I wanted to do. Anyways... if you have time and want to make it general function that can be used across all those prrojects be my guest :-) I actually need something for all other type of services... |
I know this feel. A few years ago (on my prev work), I was very pleased that powershell missed errors and continued to work. Urgent installation of patches on all working computers, distribution of the configuration file to access to the central database, search for an event in one of the logs or any other disaster on 15k computers should not stop because one computer is unavailable. But in the case of a domain change report, ignoring errors is an illusion of reliability, imho. If the script is launched from the task scheduler and works to the end ignoring errors, how do you know that errors occurred? In this case, I think the "Fail Fast" approach is very important.
|
Well then to be honest there's more to work this out if put your perspective like that:
From other stuff that needs fixing (as I have you here):
and then all those public functions would have to be merged into one function that dynamically would build the report. I guess one would need to create each of those points as a issue and go one by one fixing those. I'm sure some other stuff can be fixed, optimized. For example there's this PSHTML project that could be used to build dynamic reports for this. Sort of summary for when you just need a peak ;-) You up for any of those ? :-D I appreciate any help I can get. I've got 7+ projects that are currently in 'active' development and only pair of hands ;p |
There's also a problem with PSEventViewer and Get-EventLogSizes that if DC or any machine that is being scanned is down it will wait for 40+ seconds on that one. I wrote this function: |
Let's do it! I think we need to enable the wiki and create a page with all these points. And what about PR? |
Sure. Send it over. The one you sent looks good. I can accept this one and you can push more changes. |
I think this is a suitable issue. In the wild it will be used like this: $Params = @{
LogPath = Join-Path $LoggerParameters.LogsDir "$([datetime]::Now.ToString('yyyy.MM.dd_hh.mm'))_ADReporting.log"
ShowTime = $LoggerParameters.ShowTime
TimeFormat = $LoggerParameters.TimeFormat
}
$Logger = Get-Logger @Params if (Test-Key $ReportOptions "ReportOptions" "KeepReports" ) {
if (-not (Test-Path $ReportOptions.KeepReportsPath -PathType Container)) {
$Success = $false
$Logger.AddErrorRecord('Path in configuration of ReportOptions.KeepReportsPath doesn''t exist.')
}
} Since the first implementation is intentionally simplified, the console output has lost several colors: Bonus we get the highlight in text editors: What do you think about this? |
Do I read this right? You want to stop using PSWritecolor and start using Get-Logger for everything? What is the benefit in the long run? Don't get me wrong. I don't take things personally. If removing PSWriteColor and putting Get-Logger in it's place is going to improve things or simplify things so be it. I just want to understand. We can even improve PSWriteColor to expand it to the needs of this and other projects. Remember I control it, so not a big deal ;-) |
The main benefit is ease of use and quick changes. What do I mean when I talk about ease of use? What do I mean when I talk about quick changes? Write-Color is a really cool cmdlet, it is worth expanding and making it more flexible, but all its power is revealed just wrapped in an additional layer of abstraction. |
Now I get it. I didn't notice you're using Write-Color beneath. I did the same for PSAutomator: https://github.com/EvotecIT/PSAutomator I went for a similar generalized approach. Take a look. And yes it's very ok to go ahead with this. |
Removed terminating error. It's possible one DC being down Whole domain being down Yet Forest being fully functional #13
@snd3r I am not sure how you test things but you should really get better testing env. Also having strict checking means you can't have "problems" or non-existing variables in your config. So you either check for the existence of every variable in config or you remove strict checking. According to best practices, it shouldn't be on production code. |
Closed by accident. After some changes it seems to work now. You really should build a bit broken Test Env. I have 1 forest with 2 domains and 4 domain controllers. I have one domain, 1 controller down. I do this to make sure I cover different scenarios. With your approach to not hide errors things would not work at all. |
My bad. |
In the first approximation, your statement seems to be true, but if you look closely - no. I think we should change the logic a bit, remove the extra validation inside the script (Test-KeyVerifyBoth() and Test-KeyVerify() funcs) and improve the input validation. You suggested this idea earlier, I suggest to return to it. Main change: PSWinReporting/Private/Test-Configuration.ps1 Line 183 in a540269
PR: #33 |
|
Your change will check for the existence of the key and will execute the script every time for each of those values. That's why I created Test-KeyVerifyBoth. Because it checks for actual value of $ReportTimes.CurrentHour.Enabled = $true/$false In other words, it also checks if $ReportTimes.CurrentHour has actual value of $true/$false and is boolean. In your case, it will always return $true at least from my testing. The way you test it now, it will just check if $ReportTimes.CurrentHour exists. And it does, always. That's why my newly introduced change. |
Also why you remove -Merge ?
It actually merges two arrays. if you don't do that it may fail. |
My mistake. Accidentally broke upstream sync and kept an outdated copy. |
Test this on PastMonth, OnDay, PastQuarter. |
Checking yes, but I mean even if the values are correct $ReportTimes.OnDay will return $True. Same as $ReportTimes.PastMonth will return $true and everything under this will execute. It's not about checking config. It's about execution of get-choosendates. |
In these cases, I check the property "Enabled": # Report Per Week
if ($ReportTimes.OnDay.Enabled) {
foreach ($Day in $ReportTimes.OnDay.Days) {
$DatesReportOnDay = Find-DatesPastWeek $Day
if ($DatesReportOnDay -ne $null) {
$Dates += $DatesReportOnDay
}
}
}
# Report Per Month
if ($ReportTimes.PastMonth.Enabled) {
# Find-DatesMonthPast runs only on 1st of the month unless -Force is used
$DatesMonthPrevious = Find-DatesMonthPast -Force $ReportTimes.PastMonth.Force
if ($DatesMonthPrevious -ne $null) {
$Dates += $DatesMonthPrevious
}
} |
Ok. It's a way to do this. But then I need logger and other stuff to address my comments from above? Or you see it differently? |
I am happy to help, but I do not understand what is missing in the current implementation. Maybe I need a case.
I cannot reproduce the problem on the version of the function with your corrections. I do not think that I can do something much better than it is now, except that a small fix of copy/past mistakes: EvotecIT/PSSharedGoods#5
I understand what you mean, but I do not understand why make these changes. $Logger = Get-Logger -ShowTime -LogPath $(Join-Path 'C:\temp' 'Current.log') |
What I did is define $Script:Variable and I am checking for user input. If there is no user input I use that instead. That's what I did. My problem if you start any function outside of main starting functions (the ones from Private folder) it will crash (unless you already run Public one), as in error out as not defined (which is true). While it's not necessarily needed for users I think it would be nice to have a fail-safe in all places so that one can run a command without getting error messages if you just want to test some function outside of your standard scenarios? Hope it's clear now? What I would like is:
PSWinReporting/Public/Start-ADReporting.ps1 Lines 16 to 37 in 6645d3b
If you move Join-Path to PSSharedGoods (and the whole check for whitespace on line 29) you can actually shorten this part to:
I was also thinking the user should be able to define the file naming convention? They may want to move logs/trim in a different way?
|
Got it. |
Looks good. |
@snd3r I know now why I've implemented my helper function. Because I changed things a bit: PSWinReporting/Private/Parameters/Script.ReportTimes.ps1 Lines 1 to 65 in 2213b99
And with your removal of my support function:
And that way it supported both scenarios without me having to play with it. I'm currently rebuilding PSWinReporting to support a dynamic building of reports, but I would like to have the old and new way of working. Do you think I can bring back my function or you have other views on this? |
The expansion of the number of supported cases leads to an increase in the amount of code and the time of its support. |
Hi @PrzemyslawKlys, I have a question about error handling.
In the powershell, the error handler only works if the ErrorAction flag is set, or the global variable ErrorActionPreference is set to 'Stop'.
In this cases, error code handling doesn't work.:
PSWinReporting/Private/Get-EventLogSize.ps1
Line 11 in cba6408
PSWinReporting/Private/Test-Prerequisite.ps1
Line 51 in cba6408
I think the good way is to remove all the 'ErrorAction' arguments and set the global variable ErrorActionPreference.
This way will help us catch two birds with one stone:
The text was updated successfully, but these errors were encountered: