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

Implemented full support for GlobalQuarantinePolicy #3312

Merged
merged 6 commits into from
May 17, 2023

Conversation

max-freshmethod
Copy link
Contributor

Pull Request (PR) description

Updated Microsoft365DSC.EXOQuarantinePolicy to properly support the import and export of GlobalQuarantinePolicy with all parameters. The changes are needed because GlobalQuarantinePolicy has a different set of parameters compared to the regular QuarantinePolicy

This Pull Request (PR) fixes the following issues

Fixes #3285

@max-freshmethod
Copy link
Contributor Author

@microsoft-github-policy-service agree

Copy link
Collaborator

@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.

Please review comments.

CHANGELOG.md Outdated
@@ -39,7 +39,7 @@
* Initial release
FIXES [#3253](https://github.com/microsoft/Microsoft365DSC/issues/3253)
* EXOQuarantinePolicy
* Support exporting global quarantine policy
* Support exporting and importing global quarantine policy
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add this under the unreleased section above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

QuarantinePolicyType = $QuarantinePolicy.QuarantinePolicyType
}
}
else
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need an else statement here? What would be the issue with passing in QuarantinePolicyType for all scenarios, even if it's not equal to 'GlobalQuarantineTag'? The Get-TargetResource should then be handling it correctly no matter what.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, Nik. I removed the redundant if statement since it is not necessary there

@@ -17,4 +17,7 @@ class MSFT_EXOQuarantinePolicy : OMI_BaseResource
[Write, Description("Username can be made up to anything but password will be used for CertificatePassword"), EmbeddedInstance("MSFT_Credential")] String CertificatePassword;
[Write, Description("Path to certificate used in service principal usually a PFX file.")] String CertificatePath;
[Write, Description("Managed ID being used for authentication.")] Boolean ManagedIdentity;
[Write, Description("The EndUserSpamNotificationFrequency parameter species how often quarantine notifications are sent to users. Valid values are: 04:00:00 (4 hours),1.00:00:00 (1 day),7.00:00:00 (7 days)")] String EndUserSpamNotificationFrequency;
[Write, Description("The QuarantinePolicyType parameter filters the results by the specified quarantine policy type. Valid values are: QuarantinePolicy, GlobalQuarantinePolicy")] String QuarantinePolicyType;
[Write, Description("This parameter is reserved for internal Microsoft use.")] String EndUserSpamNotificationFrequencyInDays;
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like we are missing a few parameters e.g., CustomDisclaimer. We need to make sure that all new parameters are defined in the schema.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added missed parameters.

@ricmestre
Copy link
Contributor

@max-freshmethod Hi, it looks good to me even though unfortunately cannot test it since I don't have any windows machine available right now, but please change the mockups I added to the tests on my previous PR to return GlobalQuarantineTag instead of DefaultGlobalPolicy.

@NikCharlebois NikCharlebois merged commit ee4bd05 into microsoft:Dev May 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

EXOQuarantinePolicy not including Global Policy
3 participants