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

AADMSGroupLifecyclePolicy #447

Merged
merged 5 commits into from
Apr 8, 2020
Merged

AADMSGroupLifecyclePolicy #447

merged 5 commits into from
Apr 8, 2020

Conversation

NikCharlebois
Copy link
Collaborator

@NikCharlebois NikCharlebois commented Apr 7, 2020

Pull Request (PR) description

Initial Release of AADMSGroupLifecyclePolicy to manage Azure AD Groups' Expiration.

This Pull Request (PR) fixes the following issues

N/A


This change is Reviewable

@NikCharlebois NikCharlebois self-assigned this Apr 7, 2020
@NikCharlebois NikCharlebois added the Enhancement New feature or request label Apr 7, 2020
@NikCharlebois NikCharlebois added this to the Sprint 7 milestone Apr 7, 2020
@codecov-io
Copy link

Codecov Report

Merging #447 into Dev will increase coverage by 0%.
The diff coverage is 94%.

Impacted file tree graph

@@          Coverage Diff           @@
##             Dev    #447    +/-   ##
======================================
  Coverage     90%     90%            
======================================
  Files        108     109     +1     
  Lines      11549   11651   +102     
  Branches      10      10            
======================================
+ Hits       10506   10602    +96     
- Misses      1033    1039     +6     
  Partials      10      10            

@NikCharlebois
Copy link
Collaborator Author

@ykuijs Can I please get a code review

@NikCharlebois NikCharlebois requested a review from ykuijs April 8, 2020 12:56
Copy link
Member

@ykuijs ykuijs left a comment

Choose a reason for hiding this comment

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

Few comments

Reviewed 7 of 10 files at r1.
Reviewable status: 7 of 10 files reviewed, 5 unresolved discussions (waiting on @NikCharlebois and @ykuijs)


Modules/Office365DSC/DSCResources/MSFT_AADMSGroupLifecyclePolicy/MSFT_AADMSGroupLifecyclePolicy.psm1, line 147 at r2 (raw file):

            $emails += $email + ";"
        }
        if ($emails.EndsWith(';'))

This can be done much easier with TrimEnd(";"). You don't need the if statement in that case.


Modules/Office365DSC/DSCResources/MSFT_AADMSGroupLifecyclePolicy/MSFT_AADMSGroupLifecyclePolicy.psm1, line 166 at r2 (raw file):

            $emails += $email + ";"
        }
        if ($emails.EndsWith(';'))

Same comments as above


Modules/Office365DSC/DSCResources/MSFT_AADMSGroupLifecyclePolicy/MSFT_AADMSGroupLifecyclePolicy.psm1, line 221 at r2 (raw file):

    $CurrentValues = Get-TargetResource @PSBoundParameters

    Write-Verbose -Message "Current Values: $(Convert-O365DscHashtableToString -Hashtable $CurrentValues)"

The Get method already displays the current values


Modules/Office365DSC/DSCResources/MSFT_AADMSGroupLifecyclePolicy/MSFT_AADMSGroupLifecyclePolicy.schema.mof, line 8 at r2 (raw file):

    [Required, Description("This parameter allows the admin to select which office 365 groups the policy will apply to. 'None' will create the policy in a disabled state. 'All' will apply the policy to every Office 365 group in the tenant. 'Selected' will allow the admin to choose specific Office 365 groups that the policy will apply to."), ValueMap{"All","None", "Selected"}, Values{"All","None", "Selected"}] String ManagedGroupTypes;
    [Required, Description("Notification emails for groups that have no owners will be sent to these email addresses.")] String AlternateNotificationEmails[];
    [Write, Description("Specify if the Azure AD Groups Naming Policy should exist or not."), ValueMap{"Present","Absent"}, Values{"Present","Absent"}] String Ensure;

Needs to get updated to lifecycle policy instead of naming policy


Tests/Unit/Stubs/Generic.psm1, line 62 at r2 (raw file):

}

function Set-AzureADMSGroupLifecyclePolicy

What about the other cmdlets like New and Remove??

Copy link
Collaborator Author

@NikCharlebois NikCharlebois left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 10 files at r1, 2 of 2 files at r2.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @ykuijs)


Modules/Office365DSC/DSCResources/MSFT_AADMSGroupLifecyclePolicy/MSFT_AADMSGroupLifecyclePolicy.psm1, line 147 at r2 (raw file):

Previously, ykuijs (Yorick Kuijs) wrote…

This can be done much easier with TrimEnd(";"). You don't need the if statement in that case.

Done.


Modules/Office365DSC/DSCResources/MSFT_AADMSGroupLifecyclePolicy/MSFT_AADMSGroupLifecyclePolicy.psm1, line 166 at r2 (raw file):

Previously, ykuijs (Yorick Kuijs) wrote…

Same comments as above

Done.


Modules/Office365DSC/DSCResources/MSFT_AADMSGroupLifecyclePolicy/MSFT_AADMSGroupLifecyclePolicy.psm1, line 221 at r2 (raw file):

Previously, ykuijs (Yorick Kuijs) wrote…

The Get method already displays the current values

Done.


Modules/Office365DSC/DSCResources/MSFT_AADMSGroupLifecyclePolicy/MSFT_AADMSGroupLifecyclePolicy.schema.mof, line 8 at r2 (raw file):

Previously, ykuijs (Yorick Kuijs) wrote…

Needs to get updated to lifecycle policy instead of naming policy

Done.


Tests/Unit/Stubs/Generic.psm1, line 62 at r2 (raw file):

Previously, ykuijs (Yorick Kuijs) wrote…

What about the other cmdlets like New and Remove??

They are in the O365 stub file and were generated dyamically. Somehow the Set did not expose the proper parameters.

Copy link
Member

@ykuijs ykuijs left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 2 of 2 files at r3.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@NikCharlebois NikCharlebois merged commit 6fa01a8 into microsoft:Dev Apr 8, 2020
@NikCharlebois NikCharlebois deleted the AADMSGroupLifecyclePolicy branch April 8, 2020 20:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants