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

EXOJournalRule - New Resource #1326

Merged
merged 5 commits into from
Jul 21, 2021

Conversation

NikCharlebois
Copy link
Collaborator

@NikCharlebois NikCharlebois commented Jul 19, 2021

Pull Request (PR) description

Initial Release of EXOJournalRule

This Pull Request (PR) fixes the following issues


This change is Reviewable

@NikCharlebois
Copy link
Collaborator Author

@ykuijs Can you please do a code review on this new resource? Thanks

@NikCharlebois NikCharlebois added Enhancement New feature or request Exchange Online labels Jul 19, 2021
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 minor comments

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


Modules/Microsoft365DSC/DSCResources/MSFT_EXOJournalRule/MSFT_EXOJournalRule.psm1, line 226 at r2 (raw file):

    if ($Ensure -eq 'Present' -and $currentValues.Ensure -eq 'Absent')
    {
        Write-Verbose -Message "Creating new Journal Rule {$Name}"

Maybe add the to be configured value, just like we do in the Set section

"to {$enabledValue}"


Modules/Microsoft365DSC/DSCResources/MSFT_EXOJournalRule/MSFT_EXOJournalRule.psm1, line 244 at r2 (raw file):

            if ($Enabled -eq $true)
            {
                Write-Host "TADA!!!"

Probably some testing still in here 😄


Modules/Microsoft365DSC/DSCResources/MSFT_EXOJournalRule/MSFT_EXOJournalRule.psm1, line 256 at r2 (raw file):

    elseif ($Ensure -eq 'Absent' -and $currentValues.Ensure -eq 'Present')
    {
        Remove-JournalRule -Identity $Name | Out-Null

Add Verbose message that the rule will be removed


Modules/Microsoft365DSC/DSCResources/MSFT_EXOJournalRule/MSFT_EXOJournalRule.schema.mof, line 6 at r2 (raw file):

{
    [Key, Description("Name of the Journal Rule")] String Name;
    [Key, Description("The JournalEmailAddress parameter specifies a recipient object to which journal reports are sent. You ean use any value that uniquely identifies the recipient.")] String JournalEmailAddress;

Typo in "You can", ean must be can

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.

Reviewable status: 8 of 10 files reviewed, 4 unresolved discussions (waiting on @ykuijs)


Modules/Microsoft365DSC/DSCResources/MSFT_EXOJournalRule/MSFT_EXOJournalRule.psm1, line 226 at r2 (raw file):

Previously, ykuijs (Yorick Kuijs) wrote…

Maybe add the to be configured value, just like we do in the Set section

"to {$enabledValue}"

Done.


Modules/Microsoft365DSC/DSCResources/MSFT_EXOJournalRule/MSFT_EXOJournalRule.psm1, line 244 at r2 (raw file):

Previously, ykuijs (Yorick Kuijs) wrote…

Probably some testing still in here 😄

HAHAHAHAHAH. Good find!


Modules/Microsoft365DSC/DSCResources/MSFT_EXOJournalRule/MSFT_EXOJournalRule.psm1, line 256 at r2 (raw file):

Previously, ykuijs (Yorick Kuijs) wrote…

Add Verbose message that the rule will be removed

Done.


Modules/Microsoft365DSC/DSCResources/MSFT_EXOJournalRule/MSFT_EXOJournalRule.schema.mof, line 6 at r2 (raw file):

Previously, ykuijs (Yorick Kuijs) wrote…

Typo in "You can", ean must be can

Nice this is taking from docs.microsoft.com. I'll send them a PR to fix it up there too. Thanks

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.

Reviewed 1 of 2 files at r3.
Reviewable status: 9 of 10 files reviewed, 1 unresolved discussion (waiting on @ykuijs)

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 1 of 1 files at r4.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @NikCharlebois)

@NikCharlebois NikCharlebois merged commit 0bd9084 into microsoft:Dev Jul 21, 2021
@NikCharlebois NikCharlebois deleted the EXOJournalRule branch July 21, 2021 11:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature or request Exchange Online
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ExchangeOnline: M365DSC does not collect Journal rules EXOJournalRule
2 participants