Adds new sample "find-obsolete-m365-groups"#6250
Adds new sample "find-obsolete-m365-groups"#6250tmaestrini wants to merge 18 commits intopnp:mainfrom
Conversation
tmaestrini
changed the title
|
Cool! Thank you @tmaestrini! We'll try to review it ASAP! |
milanholemans
left a comment
milanholemans
left a comment
There was a problem hiding this comment.
Really nice script! I saw a few small things we should take a look at. Also, on my (small) tenant, the script seems to be crashing every time.
milanholemans
left a comment
milanholemans
left a comment
There was a problem hiding this comment.
Did another review and zoomed in a bit on the error I was getting @tmaestrini.
|
Did a bunch of changes, @milanholemans – thanks for your review. 🙏 I've really appreciated it. |
When ready, you can hit the "ready for review" at the bottom of the page, which I just did for you :) |
|
Hey @milanholemans, sorry for my confusion on my last commit (#b2ae47f). I wanted to open a new PR related to #6272. |
|
@milanholemans This one still waits for a review / approval. |
milanholemans
left a comment
milanholemans
left a comment
There was a problem hiding this comment.
Hi @tmaestrini, I was able to do another review and made some additional remarks. Also the script is still failing on my tenant, so let's try to get that fixed first.
| $guests = $users | Where-Object { $_.id -in $Script:Guests.id } | ||
|
|
||
| # Modify the $members list to only contain users that are not in the $owners list | ||
| $members = Compare-Object $members $owners -PassThru |
There was a problem hiding this comment.
This piece of code is still failing for all company groups which have no owners and no members.
There was a problem hiding this comment.
You're absolutely right. This would be the case, when every group member has left the company. In this case, any comparison would be obsolete.
There was a problem hiding this comment.
As mentioned in the previous review. When you have an all company group, it doesn't have any owners or members.
There was a problem hiding this comment.
@milanholemans That's why I've implemented this on line 170:
if($null -ne $owners -and $null -ne $members) {
$members = Compare-Object $members $owners -PassThru
}
Doesn't it show up in the code?
There was a problem hiding this comment.
The last time I reviewed it wasn't there. I'll recheck in my next review.
… console with unnecessary output
|
Hey @milanholemans, did a bunch of updates… 😄 Hopefully the script is not failing anymore on your tenant. |
|
Any news on this? Do you want me to change anything? |
|
Hi @tmaestrini will try to do another review soon. Due to lack of time this has not happened yet. |
milanholemans
left a comment
milanholemans
left a comment
There was a problem hiding this comment.
Seems like I'm still struggling to make the script work on my tenant. Let's have another look at it.
|
|
||
| $spoSite = $Script:TeamSites | Where-Object { $_.GroupId -eq "/Guid($($Group.Reference.id))/" } | ||
| $spoWeb = m365 spo web get --url $spoSite.Url | ConvertFrom-Json | ||
| if ($spoWeb.LastItemUserModifiedDate -lt $WarningDate) { |
There was a problem hiding this comment.
As discussed before, let's remove the spo web get request and just check the date on the spoSite object to avoid permission issues.
|
|
||
| if ($owners.Count -eq 0) { | ||
| Write-Host " → potentially obsolete (abandoned group: no owner)" -ForegroundColor Yellow | ||
| $reason = "Low user count" |
There was a problem hiding this comment.
| $reason = "Low user count" | |
| $reason = "Low owner count" |
| return | ||
| } | ||
|
|
||
| if ($owners.Count -le 1 -and ($members.Count + $guests.Count) -eq 0) { |
There was a problem hiding this comment.
Does it matter whether there's only 1 owner? I think every group without a member has some problems no? Let's also personalize the reasons, right now we're always logging "low user count".
| try { | ||
| $Global:ObsoleteGroups.Add($Group.Reference.id, $Group) | ||
| } | ||
| catch { Write-Information "Group was already added to the list of potentially obsolete groups" } |
There was a problem hiding this comment.
| catch { Write-Information "Group was already added to the list of potentially obsolete groups" } | |
| catch { } |
|
Due to a change on my fork (which caused a mess in PR #6445), I had to rename the old 'main' branch. Therefore this PR was closed automatically. Could you please reopen it, @milanholemans – or should I implement the changes and make another PR out of it? |
|
I cannot reopen it since your branch doesn't exist anymore. |
|
So, I will make another PR out of it. |
2 participants
This script is based on the original article written by Tony Redmond and was requested a long time ago in #2475.
It generates a report of all M365 groups that are possibly obsolete.
Closes #2475