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

Update MSFT_IntuneAppProtectionPolicyiOS.psm1 - fixes #2942 #2944

Closed
wants to merge 0 commits into from

Conversation

menswearUK
Copy link
Contributor

added New-M365DSCConnection command to address #2942 - added formatting to current/target values output

Pull Request (PR) description

This PR adds a line:
$ConnectionMode = New-M365DSCConnection -Workload 'MicrosoftGraph' -ProfileName beta `
-InboundParameters $PSBoundParameters

to the Test-TargetResource function of IntuneAppProtectionPolicyiOS

It prevents the Get-TargetResource function returning objects with missing values. It's not clear to me why the same line in get-TargetResource doesn't work correctly but without this line added here the object returned from the MS graph command uses the v1.0 profile and is missing a number of values which are required for the returned array. This leads to failed comparisons with config documents even when the settings are correct.

The error and the fix can be seen in a verbose output of start-DSCConfiguration or in test-DSCConfiguration

This Pull Request (PR) fixes the following issues

Fixes #2942

@ykuijs
Copy link
Member

ykuijs commented Feb 28, 2023

Even though this solution works, it is more like a workaround. I rather try to solve the underlying issue before implementing a workaround.

@menswearUK
Copy link
Contributor Author

I thought it was easiest to update this PR with the places I added code into New-M365DscConnection - since the PR is on hold and, once we reach a solution, I can amend further as necessary.
In addition to my initial suggestion I also added a check that the New-M365DscConnection was for MSGraph before applying the switch as it could be used for multiple connections.
In my testing there was no requirement to add further verbose settings changes if the command is run within New-M365DscConnection but I can add those in too if they are required

@menswearUK
Copy link
Contributor Author

@ykuijs - I've updated this PR with the fixes discussed in the issue chat - the only thing I've not done is added in the changes to the verbose output (it only seems to be an issue when the profile switch is in the configuration, it doesn't seem to be required in this function although I can add those lines in if you'd prefer).

@ykuijs ykuijs removed the On-Hold label Mar 7, 2023
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.

Thanks for this update. Just a few minor comments.

CHANGELOG.md Outdated
@@ -1,6 +1,9 @@
# Change log for Microsoft365DSC

# UNRELEASED
* New-M365DSCConnection in M365DSCUtil.psm1
Copy link
Member

Choose a reason for hiding this comment

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

Can you please move this item under the MISC section (currently on line 14)

@@ -1080,8 +1080,8 @@ function Test-TargetResource

$CurrentValues = Get-TargetResource @PSBoundParameters

Write-Verbose -Message "Current Values: $(Convert-M365DscHashtableToString -Hashtable $CurrentValues)"
Write-Verbose -Message "Target Values: $(Convert-M365DscHashtableToString -Hashtable $PSBoundParameters)"
Write-Verbose -Message "Current Values: $((Convert-M365DscHashtableToString -Hashtable $CurrentValues) -replace ';', "`r`n")"
Copy link
Member

Choose a reason for hiding this comment

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

Can you leave this as it was? I like the change, but now it is only implemented for this resource and not for all other resources. I rather change the Convert-M365DscHashtableToString function to do this, so all resources replace the semi colon with an enter.

Please create an issue for this so we can assess the impact and implement separately.

Modules/Microsoft365DSC/Modules/M365DSCUtil.psm1 Outdated Show resolved Hide resolved
@ykuijs ykuijs mentioned this pull request Mar 7, 2023
@menswearUK
Copy link
Contributor Author

@ykuijs No worries, I've corrected those values now.

@ykuijs
Copy link
Member

ykuijs commented Mar 7, 2023

@menswearUK, thanks for that. But what about the other two comments?

@menswearUK
Copy link
Contributor Author

sorry, missed those - will sort it now

@menswearUK
Copy link
Contributor Author

@ykuijs I've updated those now, Tweaked the changelog description to better match the other entries under MISC, hope it's ok?

@menswearUK menswearUK requested review from ykuijs and removed request for NikCharlebois March 7, 2023 11:14
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

@ykuijs
Copy link
Member

ykuijs commented Mar 9, 2023

@menswearUK, wanted to merge it, so I could include it in this weeks release. But unfortunately I was unable to. Could you update the PR with the Dev branch?

@menswearUK
Copy link
Contributor Author

@ykuijs - apologies for this, I was attempting to clear the conflicts and messed it up - I ended up clearing the commits and syncing and it closed the PR with no changes. I've created a new branch with the updates which has no conflicts and I've put in a new PR for that.

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.

IntuneAppProtectionPolicyIOS - Test-TargetResource returns false even if config matches existing policy
2 participants