Sync-GSCourse #338
Sync-GSCourse #338
Conversation
INitial commit of sync-gscourse
There was a problem hiding this comment.
We don't use Classroom at my institution (or at least, if we do, I'm not involved in it), so I don't have any experience here, but reading through the code it all makes sense, and the fact that you're most likely using it already means it's functional.
The one concern I have is that it seems like there's a potential for massive damage with this. You've accounted for that somewhat by setting SupportsShouldProcess and ConfirmImpact="High" but that will only impact Remove-GSCourseParticipant and Export-CSV (once the -Confirm:$Confirm issue is taken care of). The other functions that create a course, update a course, or add participants don't have SupportShouldProcess so they won't ask for Confirmation, but even worse, if the function is called with -WhatIf those commands will be executed. I would consider wrapping them all with $PSCmdlet.ShouldProcess This is a really good article describing how to do that.
You can also look at how ShouldProcess is implemented here in a function like Remove-GSCourse. Though a note of warning there, It only has a single string passed in, which defines the text for the "Target" part of the message, which results in odd output like this:
What if: Performing the operation "Remove-GSCourse" on target "Removing Course '123456'".
So, it's not a perfect example to follow 😁. It's perfectly functional, just leaves a bit to be desired style wise.
| @('123456789', 'd:Example2', 'Example3', 'Example4') | ForEach-Object { | ||
| New-Object PSObject -property @{ | ||
| ID = $_.ID |
There was a problem hiding this comment.
I think either each element in the array needs to be a hashtable with the value being an id (@(@{ID='123456789'},@{ID='d:Example2'}) etc, or the assignment of the ID just needs to be ID = $_
| Trap { | ||
| $_ | ||
| write-host "Error" | ||
| } |
There was a problem hiding this comment.
I'm not too familiar with traps, so I'm not sure what this will be doing here. In playing around, it looks like when an Error is trapped it will still get written out, so I'm not sure if anything is gained by outputting the error again. It looks like most commands that could generate errors are wrapped in try-catch blocks. Is this a remnant of the development process, or are there areas of the code that could potentially trip this?
| If ($RemoveStudents){ | ||
| Try { | ||
| $MembershipUpdated = $True | ||
| Remove-GSCourseParticipant -CourseId $CourseAlias -Student $StudentsToRemove -Confirm:$Confirm -ErrorAction Stop -Verbose:$Verbose | Out-Null |
There was a problem hiding this comment.
I don't think there's an automatic variable of $Confirm and it's not being set anywhere. I think the intent here, along with SupportsShouldProcess in the CmdletBinding is to pass through confirmation, but I don't think this will work. Since $Confirm doesn't exist, I believe it will get cast to $false here so in effect you're always disabling confirmation.
I think if you just take -Confirm:$Confirm out on these 3 commands, it will act like you want. Which, I believe is that if you execute Sync-GSCourse it will ask for confirmation in these 3 cases (the two instances of Remove, and the Export-CSV), and if you run Sync-GSCourse with -Confirm:$false then the 3 commands won't prompt for confirmation.
There was a problem hiding this comment.
Looks like the variable $ConfirmPreference gets set, according to that article I linked in the main comment. You can also add -WhatIf:$WhatIfPreference to these commands to be sure the WhatIf is passed through.
|
|
||
|
|
||
|
|
||
| If ($PassThru.IsPresent){ |
There was a problem hiding this comment.
The "IsPresent" isn't neccessary, if PassThru is specified, $PassThru will be $true, and if PassThru is not specified, $PassThru will be $false.
|
@FISHMANPET Thanks for the feedback. This is something that I was working on to replace an existing script that does the same thing. As with all things the existing script, although complicated and messy works just fine and other priorities have since come up. So truthfully I have not looked at this since I put in the initial PR to see if this is something that might be well received for PSGSuite. If you think this is something worth contributing to the module though I will review your feedback and make some changes in the coming weeks. I wouldn't plan to merge this soon though as it will take me some time to work on it. Thanks! |
|
@Foggy2 Hey, I know that feeling, I got added as a contributor here 2 years ago and then never had time for anything 😁. I think this could be useful, but I'll remove it from the 2.37.0 Milestone and give you time to come back and incorporate the feedback. |
Sync-GSCourse function per issue #334.