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.

Sign up for GitHub

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

Jump to bottom

MSFT_IntuneAppProtectionPolicyAndroid.psm1 remove Get-M365DSCIntuneAppProtectionPolicyAndroid function and fix #1955 #2091

Merged
merged 16 commits into from
Jul 25, 2022
Merged

MSFT_IntuneAppProtectionPolicyAndroid.psm1 remove Get-M365DSCIntuneAppProtectionPolicyAndroid function and fix #1955 #2091

merged 16 commits into from
Jul 25, 2022

Conversation

menswearUK
Copy link
Contributor

Pull Request (PR) description

This change amends the get-targetresource function to eliminate function Get-M365DSCIntuneAppProtectionPolicyAndroid.
The function is not needed as we already gather all the information required when we call Get-MgDeviceAppManagementAndroidManagedAppProtection.

I've added a function to gather a list of the parameters and any amendments required to allow the export function to work correctly.

Get-MgDeviceAppManagementAndroidManagedAppProtection is using the beta profile. Whilst this isn't required for this update the function I've removed used the beta profile for gathering data and I'll need it for future updates as there are some values not returned by using v1.0.

It also no longer returns the parameters which cause error #1955. I've tested the export function and have been able to use the config as exported to create a new policy

This Pull Request (PR) fixes the following issues

fixes #1955

@menswearUK
initial update to get-targetresource
42f9583 
add appgrouptype variable (doesn't actualy set it yet
remove calls to Get-M365DSCIntuneAppProtectionPolicyAndroid and corresponding function
add get-InputParameters function to allow easy looping of all parameters in other functions
@menswearUK
removed appgrouptype parameter
8501411 
removed appgrouptype parameter- that's not what I'm trying to implement here - will do that when I alter set command
@menswearUK
Update MSFT_IntuneAppProtectionPolicyAndroid.psm1
044ba19 
amended policy.add values so they acutally work
@menswearUK
changed get-InputParameters
f56a0cd 
the output using the graph module doesn't return objects that can be exported to  a DSC config - rather than try to amend reverse DSC to handle the objects (it would mean either a generic action adding or support for lots of object types) I added conversion data to this module - get-InputParameters now returns a hashtable of hashtables - I hope to be able to re-use this function for further updates.
Tested with an add and amend and it performed as expected
@menswearUK
Merge branch 'microsoft:Dev' into GraphModulesTry2
daa97bb
@menswearUK
neatened up
e904f6d 
removed commented lines and removed name value from hashtable of parameters (primary hashtable already contains name)
@menswearUK
amended tests file
0702ffd 
amended test file to remove reference to Get-M365DSCIntuneAppProtectionPolicyAndroid and to return a suitable object for Get-MgDeviceAppManagementAndroidManagedAppProtection
@menswearUK menswearUK changed the title MSFT_IntuneAppProtectionPolicyAndroid.psm1 remove - fix #1955 MSFT_IntuneAppProtectionPolicyAndroid.psm1 remove Get-M365DSCIntuneAppProtectionPolicyAndroid function and fix #1955 Jul 13, 2022
@NikCharlebois
Copy link
Collaborator

@menswearUK the parameters you are trying to remove are being returned by the function:
image
I believe the fix should be to add those to the .schema.mof file instead of trying to remove them from the .psm1 file

@menswearUK
Copy link
Contributor Author

Hi Nik,
I'd removed them as we had been having some trouble getting the JSON configuration to work with those values in the iOS configuration and didn't want to generate the same problem with the android config - I think that's all sorted now so I should be able to add them in.

I don't think isAssigned is required as the value is automatically set based on the content of assignments

Can you advise if the other code changes are acceptable? If so I'll add in those parameters to this branch

@NikCharlebois
Copy link
Collaborator

All other changes make total sense. Let's add back the values and keep IsAssigned in there as to not cause any breaking changes. Thanks

@menswearUK
Updated parameters
b9e69bb 
Added in new parameters - also discovered that assignments and excluded groups are not amended when an existing policy is amended, added this in
removed mandatory requirement for assignments value as a null array will cause errors and you may conceivably want to apply an edit to remove all assignments
I'm still not sure about isassigned as I've had to hack it in the test-targetresource
@menswearUK
Copy link
Contributor Author

Hi Nik,
I've added the parameters in now - they are functioning ok and I've added them to the tests file. I also found I needed to tweak the set-target resource function as it was not setting the assignments and excluded groups when amending a policy, only when creating.
I've added IsAssigned but it doesn't appear to do much - the value sets itself based on the assignments and it doesn't matter what you specify in the config (although it doesn't cause any errors either, it just doesn't set the value) - I've had to put a check in the test-targetresource so it doesn't incorrectly detect drift.

@menswearUK
Update Microsoft365DSC.IntuneAppProtectionPolicyAndroid.Tests.ps1
dcfdbd9 
tweaked tests file so returned objects and config default parameters are only set in a single place
@menswearUK
Merge branch 'microsoft:Dev' into GraphModulesTry2
99ab883
@menswearUK
Update CHANGELOG.md
29944a5 
updated changelog
@menswearUK
Copy link
Contributor Author

Hi @NikCharlebois ,
I've updated the changelog with the amendments I've made. I also tweaked the tests file so the parameters and return objects only need to be specified in one place.
Is there anything else you need me to change for this update? I'm still not sure about the isAssigned parameter as I don't really think it's doing anything, I've tested it with a few configurations and it always sets the value based on the value of assignments and ignores the configuration document. It was never included in the schema.mof for this configuration but it was exporting that value to the config docs in the initial release. It is present in this update but if you'd prefer I can remove it.

@menswearUK
Copy link
Contributor Author

@NikCharlebois
I've just realised that if the ...version values are not set they don't export - and because the set method demands a value for every parameter it'll cause a failure for configs where these values are not specified. So it'll fix one error and create another. I'll need to change the json construction so it omits null values.
I'd been intending to amend the set-targetresource function anyway but I'd hoped to do things incrementally. My ultimate intention was to use New-MgDeviceAppMgtAndroidManagedAppProtection, etc instead of the direct calls - would this be acceptable or shall I stick to constructing a json and Invoke-MgGraphRequest?

@NikCharlebois
Copy link
Collaborator

@menswearUK we should try to call into the cmdlet wherever possible instead of direct calls. Let me know when you've had a chance to refactor the PR and I'll review it. Thanks again for all your hard work!

menswearUK and others added 3 commits July 21, 2022 00:45
@menswearUK
updates to use graph module cmdlets
788e976 
added appgrouptype parameter
amended to use graph module cmdlets
removed unused functions
added a few formatting functions to  constrinct json snippets, convert timespans, and set the appgrouptype and apps values
Updated test file with mocking for graph module, etc.
amended the test-targetresource module
@menswearUK
Update CHANGELOG.md
360784b
@NikCharlebois
Merge branch 'Dev' into GraphModulesTry2
d17ccdd
NikCharlebois
NikCharlebois approved these changes Jul 22, 2022
View reviewed changes
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.

LGTM

@menswearUK
update to changelog and removal of comments
7af4b02 
updated changelog to reflect new features
minor tidy up of main code - removed commented line. commented out a write-host I used for info while testing, etc
@menswearUK
Merge branch 'microsoft:Dev' into GraphModulesTry2
590ee3a
@menswearUK
Copy link
Contributor Author

Hi @NikCharlebois ,
If this is looking ok I've just added some final amendments - nothing major, just removal of some commented lines and commenting out a write-host I used while testing. I've also added the updates into the changelog file, which I'd removed previously to clear conflicts.
Hopefully this is good to go now, if there's anything else you want adding or amending please let me know - otherwise I won't amend this branch any further. Thanks

NikCharlebois
NikCharlebois approved these changes Jul 25, 2022
View reviewed changes
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.

LGTM

@NikCharlebois NikCharlebois merged commit 3cbdaca into microsoft:Dev Jul 25, 2022
@menswearUK menswearUK deleted the GraphModulesTry2 branch July 26, 2022 09:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NikCharlebois NikCharlebois NikCharlebois approved these changes

Assignees
No one assigned
Labels
Intune
Projects
None yet
Milestone
No milestone
2 participants
@menswearUK @NikCharlebois