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

[New resource] IntuneMobileAppConfigurationPolicyIos #5401

Merged

Conversation

dannyKBjj
Copy link
Contributor

Pull Request (PR) description

This Pull Request (PR) fixes the following issues

Export appears to be working for "settings"
Module is affected by issue microsoft#5396. Replace ` with " in M365TenantConfig.ps1 is your configuration key values contain double quotes. Otherwise, it should work as expected.
@ykuijs
Copy link
Member

ykuijs commented Nov 14, 2024

Same comment as for your other PR:
All of our resources require unit tests (which are also completing successfully) and example files, for example:

Also we need an entry of this new resource in the changelog: https://github.com/microsoft/Microsoft365DSC/blob/Dev/CHANGELOG.md

Could you please add these items to the PR?

create,update and remove examples added
@dannyKBjj
Copy link
Contributor Author

Added unit test, examples and updated change log. Hopefully I did it right?!

@ykuijs
Copy link
Member

ykuijs commented Nov 15, 2024

Do not see the updated changelog yet. Could you please check?

Comment on lines 6 to 30
"delegated": {
"read": [
{
"name": "DeviceManagementApps.Read.All"
}
],
"update": [
{
"name": "DeviceManagementApps.ReadWrite.All"
}
]
},
"application": {
"read": [
{
"name": "DeviceManagementApps.Read.All"
}
],
"update": [
{
"name": "DeviceManagementApps.ReadWrite.All"
}
]
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Because we're dealing with Intune assignments, an additional entry for Group.Read.All is required. You can take at the other Intune resources.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should be resolved in latest pull request

 Group.Read.All permission added to settings.json

Excessive number of blank lines reduced for readability.
@dannyKBjj
Copy link
Contributor Author

Do not see the updated changelog yet. Could you please check?

should be resolved in latest pull request

Copy link
Contributor

@FabienTschanz FabienTschanz left a comment

Choose a reason for hiding this comment

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

@dannyKBjj You don't need a separate pull request just for the changelog, adding it here (just as you did) is fine. Although you can omit the MSFT_ prefix in front of the resource name and place it in order 😃

dannyKBjj and others added 4 commits November 19, 2024 15:51
…gurationPolicyIOS/MSFT_IntuneMobileAppConfigurationPolicyIOS.psm1

Co-authored-by: FabienTschanz <[email protected]>
…gurationPolicyIOS/MSFT_IntuneMobileAppConfigurationPolicyIOS.psm1

Co-authored-by: FabienTschanz <[email protected]>
CHANGELOG.md Outdated
@@ -23,6 +23,8 @@
* DEPENDENCIES
* Updated DSCParser to version 2.0.0.12.
* Updated MSCloudLoginAssistant to version 1.1.28.
* MSFT_IntuneMobileAppConfigurationPolicyIOS
Copy link
Member

Choose a reason for hiding this comment

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

Please remove the MSFT_ prefix and then place this entry in alphabetical order in the list

@dannyKBjj
Copy link
Contributor Author

Fixed 2-Update.ps1 example. Should now pass unit tests.

@dannyKBjj
Copy link
Contributor Author

Unsure how best to resolve CHANGELOG.md conflict?

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 ykuijs changed the title Msft intune mobile app configuration policy ios [New resource] IntuneMobileAppConfigurationPolicyIos Nov 20, 2024
@ykuijs ykuijs merged commit 6709714 into microsoft:Dev Nov 20, 2024
2 checks passed
@dannyKBjj
Copy link
Contributor Author

dannyKBjj commented Dec 19, 2024

Hi, see issue #5587 as I believe this module fixes the issue. Have created pull request to re-instate my module and remove the one that this duplicated.

Android apps are handled by the module IntuneAndroidManagedStoreAppConfiguration (which I also submitted), but for some reason this was never removed.

Issue #5587 for module "IntuneAppConfigurationDevicePolicy" seems to be caused by the logic the combined resource uses to determine the application assignment. The separated resources do not have this problem.

@dannyKBjj dannyKBjj mentioned this pull request Dec 19, 2024
7 tasks
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.

3 participants