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

Missing Ensure Parameters for various resources #2718

Closed
andikrueger opened this issue Dec 29, 2022 · 8 comments · Fixed by #2872
Closed

Missing Ensure Parameters for various resources #2718

andikrueger opened this issue Dec 29, 2022 · 8 comments · Fixed by #2872
Assignees

Comments

@andikrueger
Copy link
Collaborator

andikrueger commented Dec 29, 2022

Some resources do not have an Ensure parameter. Some of these resource can only be used once in a configuration.

  • AADTenantDetails

  • EXOOrganizationConfig (creates wrong null return)

  • EXOTransportConfig (creates wrong null return)

  • PPTenantSettings

  • PPTenantIsolationSettings

  • SPOBrowserIdleSignout

  • SPOSiteAuditSettings

  • SPOTenantCDNPolicy

  • TeamsDialInConferencingTenantSettings

  • TeamsFederationConfiguration

  • TeamsGuestCallingConfiguration

  • TeamsGuestMeetingConfiguration

  • TeamsGuestMessagingConfiguration

  • TeamsMeetingBroadcastConfiguration

  • TeamsUpgradeConfiguration

  • TeamsUpgradePolicy

@ykuijs
Copy link
Member

ykuijs commented Jan 2, 2023

It is correct that some resources do not have the Ensure parameter, especially when they can only be used once. For example: With AADTenantDetails, these settings have a certain default value and cannot be removed. They can only be changed to something else. Therefore the Ensure parameter does not make sense.

To enforce that the resource can only be specified once, these resources are using the IsSingleInstance parameter.

You are correct though that some are incorrectly using the Ensure parameter in the null return. Will check those and correct where required.

@andikrueger
Copy link
Collaborator Author

There are some resources with single instance though, that have the ensure parameter with just one allowed value of „Present“. I like this use as this would be more consistent across the usage of DSC. It would also help to show which resources are non-changeable.

@ykuijs
Copy link
Member

ykuijs commented Jan 6, 2023

I have been working on this and indeed notice there are several resources that only allow Ensure to have a value of Present. But at the same time, they set the value of Ensure to Absent themselves in the Get method. That doesn't make sense and can result in issues when exporting the Ensure=Absent which is not supported when importing again.

We can either:

  1. Use a different method to specify that something is not present, like just returning the mandatory parameters and in the export method check if the number of properties matches that number. And if so, do not export. This means we have to remove these parameters (as a breaking release in April). I prefer this method, because it makes it more consistent.

For example:

    $nullReturn = @{
        IsSingleInstance = 'Yes'
    }

And then in the export:

                $Results = Get-TargetResource @Params

                if ($Results -is [System.Collections.Hashtable] -and $Results.Count -gt 1)
                {
                    $Results = Update-M365DSCExportAuthenticationResults -ConnectionMode $ConnectionMode `
                        -Results $Results
                    $currentDSCBlock = Get-M365DSCExportContentForResource -ResourceName $ResourceName `
                        -ConnectionMode $ConnectionMode `
                        -ModulePath $PSScriptRoot `
                        -Results $Results `
                        -Credential $Credential
                    $dscContent += $currentDSCBlock

                    Save-M365DSCPartialExport -Content $currentDSCBlock `
                        -FileName $Global:PartialExportFileName

                    Write-Host $Global:M365DSCEmojiGreenCheckMark
                }
                else
                {
                    Write-Host $Global:M365DSCEmojiRedX
                }
  1. Add the Absent value and in the code check if Absent is specified. When that is the case, throw an error specifying that that value is not possible.

@NikCharlebois, what do you think?

@NikCharlebois
Copy link
Collaborator

To me, resources that implement the IsSingleInstance should never be null, meaning they should always be exporter. These are normally tenant wide settings that have a default, but cannot be null. For example, the O365OrgSettings will always have values.
Am I missing something?

@andikrueger
Copy link
Collaborator Author

This was one of my thoughts too. I did not look into the anatomy of each resources above. Usually every resource should have a default setting and therefor it should not be possible to have a null return.

If there are resources like the O365OrgSetting which can only be set once without a way back, we need to figure out a way on how to represent the default return. There is no “parameter” is $null. This resource could only part of an export if it was set in the first place. Any other value is not possible - even though this would require a lot of knowledge of how this resource works internally.

@ykuijs
Copy link
Member

ykuijs commented Jan 6, 2023

Agree with that. In this case the nullReturn value is mostly returned when something goes wrong, for example when the account does not have the correct permissions.

Shall I create a PR with my suggestions, so you both can see if you agree with those changes or if we need to go into a different direction?

@NikCharlebois
Copy link
Collaborator

@ykuijs I think this is probably the best option and the easiest for us to be able to review and provide feedback. Thanks

@ykuijs ykuijs mentioned this issue Feb 3, 2023
@ykuijs
Copy link
Member

ykuijs commented Feb 3, 2023

@NikCharlebois @andikrueger I just created PR 2872 that implements suggestions for this issue. Please review it and let me know what you think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants