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

Adds new sample "find-obsolete-m365-groups" #6250

Closed
wants to merge 18 commits into from
Closed

Conversation

tmaestrini
Copy link

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

@tmaestrini tmaestrini changed the title Adds new sample find-obsolete-m365-groups in the tenant Adds new sample "find-obsolete-m365-groups" Aug 14, 2024
@milanholemans
Copy link
Contributor

Cool! Thank you @tmaestrini! We'll try to review it ASAP!

@milanholemans milanholemans self-assigned this Aug 24, 2024
Copy link
Contributor

@milanholemans milanholemans left a 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.

@milanholemans milanholemans marked this pull request as draft August 24, 2024 18:39
@milanholemans milanholemans marked this pull request as ready for review August 30, 2024 22:29
Copy link
Contributor

@milanholemans milanholemans left a 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.

@milanholemans milanholemans marked this pull request as draft September 4, 2024 18:07
@tmaestrini
Copy link
Author

Did a bunch of changes, @milanholemans – thanks for your review. 🙏 I've really appreciated it.

@milanholemans milanholemans marked this pull request as ready for review September 9, 2024 13:25
@milanholemans
Copy link
Contributor

milanholemans commented Sep 9, 2024

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 :)

@tmaestrini
Copy link
Author

Hey @milanholemans, sorry for my confusion on my last commit (#b2ae47f). I wanted to open a new PR related to #6272.

@tmaestrini
Copy link
Author

tmaestrini commented Sep 19, 2024

@milanholemans This one still waits for a review / approval.

Copy link
Contributor

@milanholemans milanholemans left a 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.

$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
Copy link
Contributor

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.

Copy link
Author

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.

Copy link
Contributor

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.

Copy link
Author

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?

Copy link
Contributor

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.

@milanholemans milanholemans marked this pull request as draft September 20, 2024 19:11
@tmaestrini
Copy link
Author

Hey @milanholemans, did a bunch of updates… 😄 Hopefully the script is not failing anymore on your tenant.
Tell me what you think. Have a good night! 😄

@tmaestrini tmaestrini marked this pull request as ready for review September 25, 2024 23:27
@tmaestrini
Copy link
Author

tmaestrini commented Oct 15, 2024

Any news on this? Do you want me to change anything?

@milanholemans
Copy link
Contributor

Hi @tmaestrini will try to do another review soon. Due to lack of time this has not happened yet.

Copy link
Contributor

@milanholemans milanholemans left a 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
Copy link
Contributor

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) {
Copy link
Contributor

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"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
$reason = "Low user count"
$reason = "Low owner count"

return
}

if ($owners.Count -le 1 -and ($members.Count + $guests.Count) -eq 0) {
Copy link
Contributor

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" }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
catch { Write-Information "Group was already added to the list of potentially obsolete groups" }
catch { }

@milanholemans milanholemans marked this pull request as draft October 15, 2024 22:16
@tmaestrini tmaestrini deleted the branch pnp:main October 22, 2024 21:00
@tmaestrini tmaestrini closed this Oct 22, 2024
@tmaestrini tmaestrini deleted the main branch October 22, 2024 21:00
@tmaestrini
Copy link
Author

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?

@milanholemans
Copy link
Contributor

I cannot reopen it since your branch doesn't exist anymore.

@tmaestrini
Copy link
Author

So, I will make another PR out of it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New sample script: Find obsolete M365 groups
2 participants