-
Notifications
You must be signed in to change notification settings - Fork 326
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
Adds new sample "find-obsolete-m365-groups" #6250
Conversation
Cool! Thank you @tmaestrini! We'll try to review it ASAP! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
docs/docs/sample-scripts/entra/find-obsolete-m365-groups/index.mdx
Outdated
Show resolved
Hide resolved
docs/docs/sample-scripts/entra/find-obsolete-m365-groups/index.mdx
Outdated
Show resolved
Hide resolved
docs/docs/sample-scripts/entra/find-obsolete-m365-groups/index.mdx
Outdated
Show resolved
Hide resolved
docs/docs/sample-scripts/entra/find-obsolete-m365-groups/index.mdx
Outdated
Show resolved
Hide resolved
docs/docs/sample-scripts/entra/find-obsolete-m365-groups/index.mdx
Outdated
Show resolved
Hide resolved
docs/docs/sample-scripts/entra/find-obsolete-m365-groups/index.mdx
Outdated
Show resolved
Hide resolved
docs/docs/sample-scripts/entra/find-obsolete-m365-groups/index.mdx
Outdated
Show resolved
Hide resolved
docs/docs/sample-scripts/entra/find-obsolete-m365-groups/index.mdx
Outdated
Show resolved
Hide resolved
docs/docs/sample-scripts/entra/find-obsolete-m365-groups/assets/sample.json
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did another review and zoomed in a bit on the error I was getting @tmaestrini.
docs/docs/sample-scripts/entra/find-obsolete-m365-groups/index.mdx
Outdated
Show resolved
Hide resolved
docs/docs/sample-scripts/entra/find-obsolete-m365-groups/index.mdx
Outdated
Show resolved
Hide resolved
docs/docs/sample-scripts/entra/find-obsolete-m365-groups/index.mdx
Outdated
Show resolved
Hide resolved
docs/docs/sample-scripts/entra/find-obsolete-m365-groups/index.mdx
Outdated
Show resolved
Hide resolved
docs/docs/sample-scripts/entra/find-obsolete-m365-groups/index.mdx
Outdated
Show resolved
Hide resolved
docs/docs/sample-scripts/entra/find-obsolete-m365-groups/index.mdx
Outdated
Show resolved
Hide resolved
docs/docs/sample-scripts/entra/find-obsolete-m365-groups/index.mdx
Outdated
Show resolved
Hide resolved
docs/docs/sample-scripts/entra/find-obsolete-m365-groups/index.mdx
Outdated
Show resolved
Hide resolved
docs/docs/sample-scripts/entra/find-obsolete-m365-groups/index.mdx
Outdated
Show resolved
Hide resolved
docs/docs/sample-scripts/entra/find-obsolete-m365-groups/index.mdx
Outdated
Show resolved
Hide resolved
docs/docs/sample-scripts/entra/find-obsolete-m365-groups/index.mdx
Outdated
Show resolved
Hide resolved
docs/docs/sample-scripts/entra/find-obsolete-m365-groups/index.mdx
Outdated
Show resolved
Hide resolved
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
docs/docs/sample-scripts/entra/find-obsolete-m365-groups/index.mdx
Outdated
Show resolved
Hide resolved
docs/docs/sample-scripts/entra/find-obsolete-m365-groups/index.mdx
Outdated
Show resolved
Hide resolved
docs/docs/sample-scripts/entra/find-obsolete-m365-groups/index.mdx
Outdated
Show resolved
Hide resolved
docs/docs/sample-scripts/entra/find-obsolete-m365-groups/index.mdx
Outdated
Show resolved
Hide resolved
$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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The last time I reviewed it wasn't there. I'll recheck in my next review.
docs/docs/sample-scripts/entra/find-obsolete-m365-groups/index.mdx
Outdated
Show resolved
Hide resolved
… 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like I'm still struggling to make the script work on my tenant. Let's have another look at it.
$tStamp = (Get-Date).ToString("yyyy-MM-dd HH:mm:ss") | ||
if ($TestPath -ne $true) { | ||
New-Item -ItemType directory -Path $Script:Path | Out-Null | ||
Write-Host "Will create file in $($Script:Path): M365GroupsReport-$tStamp.csv" -ForegroundColor Yellow |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm getting an error:
The filename, directory name, or volume label syntax is incorrect. : 'C:\Report\M365GroupsReport-2024-10-15 23:32:22.csv'
Obviously, colons are not supported as characters in Windows. Let's strip the file name from illegal characters.
|
||
$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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
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