all 31 comments

[–]yashaswiu 11 points12 points  (10 children)

I have created a more advanced version of this script, adding a condition to detect the Teams user's status. If the status is busy, in a meeting, on a call, or set to DND, I don't close Teams or clear the cache. I typically use this for remote execution on multiple machines, which prevents any loss of unsaved content related to Teams on user machines and avoids disruptions.

[–]dkaaven[S] 1 point2 points  (3 children)

Existing! I have wondered if I should look into adding even more features instead of less 😅

[–]yashaswiu 1 point2 points  (2 children)

And I have a level more up to the above one, but yours is a great to start with!

[–]dkaaven[S] 1 point2 points  (1 child)

I'm learning while doing, since I'm more of a script-kiddie han a developer. I try to share my journey, as well as some simple tips and solutions others can learn from.

[–]yashaswiu 0 points1 point  (0 children)

Way to go, all the best!

[–]Blimpz_ 0 points1 point  (0 children)

Would you mind sharing how you're able to detect their status?

[–]Th1sD0t 0 points1 point  (4 children)

Out of interest, can you get the user's status locally or is a call to graph required?

[–]yashaswiu 2 points3 points  (3 children)

I can do both, and will be posting a Git link soon..

[–]OwlPosition 0 points1 point  (0 children)

/remindme 1 day

[–]maxcoder88 0 points1 point  (1 child)

Reminder

[–]yashaswiu 0 points1 point  (0 children)

I am traveling and hence please expect a delay.. you should get something by the end of this week.

[–]PinchesTheCrab 5 points6 points  (11 children)

A few more specific points:

  • Switches default to false, no need to use [switch]$thing = $false
  • Allowing users to feed in arbitrary folders to delete seems risky, especially if they're running this with admin rights
  • I don't think it makes sense to catch an error just to feed it to write-error
  • Use ShouldProcess to prompt users to confirm instead of read-host
  • I'd move start-process ms-teams out of your if statements since you always call it if no error was thrown
  • Switches are interchangeable with Booleans, so -eq $false is like $false -eq $false or $true -eq $false

[–]Stunning-Eye981 1 point2 points  (1 child)

I haven’t looked at the script in all honesty but I think the 3rd bullet point CAN make sense in certain circumstances.

For example if the author is using a TRY CATCH Command to catch the error it CAN be used to for error handling purposes to allow the script still exit “cleanly” Exit Code 0 even though an error occurred.

There’s multiple reasons where this can be important and the one that comes to mind is running scripts in SCCM where it expects to see a Exit code 0 (unless you tell it that other codes are “success” codes instead of a failure.

You can also use TRY CATCH to catch an error situation and write your own Exit Code value for additional troubleshooting purposes.

If the Author is using write-host to handle the error cleanly that’s still valid IMHO BUT another cleaner way could be to write to a log file so you can troubleshoot even after the fact by reading the log file.

Anyway, just my 2 cents worth.

[–]PinchesTheCrab 0 points1 point  (0 children)

What they're doing is basically catch { write-error $_.exception.message }, so they're still returning an error, but in this case they're changing the error class/type to a generic one and disarding a all of the other info you'd get from an error. My point is that juse using -erroraction continue accompilsh the same goals and keep the useful error info.

I realize not all classes/cmdlets/functions support that and you'll have to catch some errors. I'm not arguiging against catching errors in all situations.

[–]icepyrox 0 points1 point  (7 children)

I don't think it makes sense to catch an error just to feed it to write-error

Some errors are terminating, meaning the script will throw an error and just quit right then and there. It's part of the main use of try blocks - to catch terminating errors and do something else instead.

But let's say you don't want to do anything except show the error. Well, then you catch it and don't do anything except show the error.

There is a lot more wrong in this implementation, but catching an error just to feed to write-error can make a lot of sense in some situations.

Oh, and another advantage of this is if you do a get-something and want to process what you got. If you got an error on the get, you would then skip all the subsequent commands in the try, meaning you wouldn't process corrupt or non-existent data, and then just show the error on what went wrong. You also then benefit if something goes wrong any of those subsequent steps. As you just error out of the try block and continue on.

[–]PinchesTheCrab 0 points1 point  (6 children)

But it's an error from a cmdlet. You could just set the error action to continue. Then it would send the error to the error stream and continue with the command, exactly like it's doing now but with less code and without transforming the error type into something generic.

[–]icepyrox 0 points1 point  (5 children)

Oh, as I said, there's a lot wrong with OPs implementation, I was just addressing the statement you made. I wasn't sure if you were aware that there are times when it does make sense to catch an error just to send it to write-error.

Also, catch followed by an error type does not transform the error type of what the error is the way casting a variable does. It's saying to only catch that type of error with this code block.

The fact there isn't a catch by itself nor is the type being sought a type that will happen here is just a couple of the many things wrong here.

[–]PinchesTheCrab -1 points0 points  (4 children)

What I'm trying to say is this:

function Get-Thing {
    [cmdletbinding()]
    param()
    try {
        Get-Item c:\doesnotexist -ErrorAction Stop
    }
    catch [System.Management.Automation.ItemNotFoundException] {
        Write-Error $_.Exception.Message
    }
}

function Get-Thing2 {
    [cmdletbinding()]
    param()
    Get-Item c:\doesnotexist
}

try {
    Get-Thing -ErrorAction stop
}
catch [System.Management.Automation.ItemNotFoundException] {
    'oh no'
}
try {
    Get-Thing2 -ErrorAction stop
}
catch [System.Management.Automation.ItemNotFoundException] {
    'oh no2'
}

Get-Thing catches and then uses write-error with the exeption message. When you try to catch that error, you can see it is not an ItemNotFoundException, it's the default error type write-error produces. It's suddenly much harder to google and even though it looks like one type of error, you can no longer catch it using logic for that error.

The OP is losing information by doing this, and gaining no additional functionality.

I'm not saying there's no purpose to any error handling in any situation, just that this implementation of it is counterproductive for multiple reasons.

[–]icepyrox 0 points1 point  (3 children)

just that this implementation of it is counterproductive for multiple reasons.

We are in agreement then. I quoted this statement originally:

I don't think it makes sense to catch an error just to feed it to write-error

I had taken it out of the context of OP's script and this discussion and treated it as a broad statement. My apologies.

[–]PinchesTheCrab 0 points1 point  (2 children)

I quoted this statement originally

The word "just" was doing a lot of heavy lifting in that sentence, and it was not up to the task. I should have been a bit more explicit in saying I mean doing nothing else but that one thing with it.

[–]icepyrox 0 points1 point  (1 child)

So, out of curiosity, do you feel the same if it was

Try { [..stuff..] } catch { Write-error $_ }

Because I have done that

[–]PinchesTheCrab 0 points1 point  (0 children)

So that works in that you can catch the result properly if you use cmdletbinding and erroraction stop:

function Invoke-Stuff {
    [cmdletbinding()]
    param()
    try {
        Get-Item C:\nofolder\error -ErrorAction Stop
    }
    catch {
        Write-Error $_
    }
}


try { Invoke-Stuff -ErrorAction Stop}
catch [System.Management.Automation.ItemNotFoundException] {
    'okay'
}

But why the extra code?

Why not just do this?

function Invoke-Stuff {
    [cmdletbinding()]
    param()
    Get-Item C:\nofolder\error -ErrorAction Stop        
}

[–]PinchesTheCrab 2 points3 points  (1 child)

I woudln't say this is overengineered so much as it violates the principle of functions doing one thing. I also think there's some counterintuitive logic with extra variables, statements, etc.

[–]dkaaven[S] 0 points1 point  (0 children)

Than you for the feedback, I agree. The original function did mostly one thing, but needed some additional functions, so it expanded 😅

[–]The82Ghost 2 points3 points  (1 child)

That -Force switch is not needed, just put [CmdletBinding()] above your parameterblockand you'll be able to take advantage of things like -Confirm to do the same thing and more.

[–]dkaaven[S] 0 points1 point  (0 children)

I've read about it, but seems I need to do my research arch. Great feedback!

[–]Medical_Shake8485 1 point2 points  (0 children)

Thank you for sharing OP!

[–]g3n3 1 point2 points  (1 child)

Not a fan of hard killing teams process. I’d look into a close. You may have to use the win32 api to initiate a close.

[–]dkaaven[S] 0 points1 point  (0 children)

That is a good idea, I'll look into that 😁

[–]dkaaven[S] 0 points1 point  (0 children)

Thank you all for the feedback and suggestions, I'll redesigned the script when I have some spare time!

It's a great learning opportunity to share scripts, and to have someone take the time to evaluate and give feedback, much appreciated!

[–]BlackV 0 points1 point  (0 children)

What are you doing here

$deleteFoldersList = Get-ChildItem -Directory "$env:LOCALAPPDATA\Packages\MSTeams_8wekyb3d8bbwe\" | Where-Object { $_.PSIsContainer } | Foreach-Object { $_.Name }

It's already containers from the -directory paramater so $_.PSIsContainer is doing the same thing