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

ExoMailboxStatistics initial release #3866

Open
wants to merge 6 commits into
base: Dev
Choose a base branch
from

Conversation

levesquesamuel
Copy link
Contributor

Pull Request (PR) description

  • ExoMailboxStatistics
    • Initial release
  • EXODistributionGroup
    • Add custom attributes properties

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.

Not sure if this resource should be implemented. It is misusing M365DSC to retrieve information from M365. M365DSC is meant to manage M365 using Configuration-as-Code practices.

@@ -0,0 +1,74 @@
[ClassVersion("1.0.0.0"), FriendlyName("EXOMailboxStatistics")]
class MSFT_EXOMailboxStatistics : OMI_BaseResource
{
Copy link
Member

Choose a reason for hiding this comment

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

The resource should have at least one Key parameter. This is currently causing failing QA tests.

[OutputType([System.Collections.Hashtable])]
param
(
########################
Copy link
Member

Choose a reason for hiding this comment

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

If this is the minimum property set, shouldn't all properties be mandatory??

class MSFT_EXOMailboxStatistics : OMI_BaseResource
{
[Write, Description("")] String DeletedItemCount;
[Write, Description("")] String DisplayName;
Copy link
Member

Choose a reason for hiding this comment

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

The DisplayName is Mandatory in the code. That means that this property should either be Required or Key in the schema.

[ClassVersion("1.0.0.0"), FriendlyName("EXOMailboxStatistics")]
class MSFT_EXOMailboxStatistics : OMI_BaseResource
{
[Write, Description("")] String DeletedItemCount;
Copy link
Member

Choose a reason for hiding this comment

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

Please add a description to each property. We are using that to generate documentation.

[Write, Description("")] String SystemMessageSizeShutoffQuota;
[Write, Description("")] String SystemMessageSizeWarningQuota;

[Write, Description("Present ensures the group exists, absent ensures it is removed"), ValueMap{"Present","Absent"}, Values{"Present","Absent"}] string Ensure;
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this description is a copy/paste. Please correct

$ManagedIdentity
)

throw 'Updating Mailbox Statistics is not supported.'
Copy link
Member

Choose a reason for hiding this comment

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

This looks very strange. You are now trying to use DSC to retrieve information from M365. That is not what DSC is meant for.

@NikCharlebois, @andikrueger, what is your opinion on this implementation?

Copy link
Member

Choose a reason for hiding this comment

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

@ykuijs, I have inherited this PR. The customer who initiated the PR uses the mailbox statistic data to control the flow of a restore. Right, a resource that only reads data is not the primary goal of but there are examples that follow a similar pattern like PendingReboot.

If you are supporting the idea in general I will make the requested code changes.

Copy link
Member

Choose a reason for hiding this comment

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

PendingReboot actually changes something, it updates the $global:DSCMachineStatus variable.

I am leaning towards a feeling that M365DSC isn't the correct place for just retrieving information, especially for information that can be a very large data set (for environments that have large amounts of mailboxes, which can be the case quickly).

@NikCharlebois, what is your opinion on this resource?
(Responses might be a little delayed, since Nik is off at the moment)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would not implement this kind of change even though it is tempting to have the additional information in the exported data. #4375 describes a similar ask for a read-only property.

@FabienTschanz
Copy link
Contributor

Please close the PR or update with the latest changes.

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.

6 participants