Fix for issue - #5587 #5588
Fix for issue - #5587 #5588
Conversation
MSFT_IntuneMobileAppConfigurationPolicyIOS and MSFT_IntuneAndroidManagedStoreAppConfiguration were supposed to be pulled in favour of existing DSCResource - IntuneAppConfigurationDevicePolicy as my modules duplicated functionality. Whilst testing on our tenant we discovered the following issue: IntuneAppConfigurationDevicePolicy cannot handle 'for Intune' applications microsoft#5587 It seems that for some reason only MSFT_IntuneMobileAppConfigurationPolicyIOS was removed. This pull request adds that module back in and deletes "IntuneAppConfigurationDevicePolicy" as I believe that module is now defunct.
|
@dannyKBjj Removing a resource is a breaking change. The next braking change is (I believe) in April 2025. It would be better to add the functionality to |
|
Hi, but the problem is that it is using convoluted logic to handle both. It is inconsistent with almost every other module (which all handle each workload separately) and has caused an issue, who is to say there aren't other issues? Is it possible to just submit my module alongside it and remove the bugged module at a later date? I don't really want to re-write the existing module as I believe the approach I've taken is better anyway (besides which, I have a working solution already). |
|
@dannyKBjj Normally, if there is an issue with a module, we fix the issue in that. The name of the module comes from the separation between the two categories "Managed App" and "Managed Devices". These are also the two things we can select from in the Intune portal, which aligns more with the user experience someone would expect. We already had some cases where we didn't select the "correct" name by workload or by the resource type of what Graph would tell us but rather stay in the Intune portal realms with their naming. It's normal for new features and functionality to appear. This happens all the time, so we have to adapt the existing resources for their new purpose. I'd wait until @ykuijs and @NikCharlebois are back and get their opinion as well. I'm just a public contributor adding my input. If you think your solution is better, we'll wait for their feedback. That's totally fine by me. |
|
Hi, fair enough... I don't think this is a new feature thing, this is just a fault in the way approach taken. The issue is that the policy pulled by Get-MgBetaDeviceAppManagementMobileAppConfiguration uses the 'ID' of the targeted mobile app in the 'TargetedMobileApps' property, which is a unique value. For reasons I don't quite understand existing IntuneAppConfigurationDevicePolicy doesn't use this ID directly, but instead uses it to retrieve the packageId/bundleId. The problem is that for these apps iOS and Android will have share the same value, see below:
It then uses value this to retrieve the 'Id' in Set-Target (see below) However, because the app/bundleId for iOS and Android is the same, it will simply retrieve both IDs and write them into the policy (which breaks it). I'll see what is decided in the new year, but from my perspective, the general approach seems to be to write a module for each resource, as DSC doesn't store the platform information. In this case the module seems to be determining it by the presence of certain properties after the fact. Just seems unnecessarily complicated and out of step with most of the other modules. I can fix the existing one, but seems fixing it is very much the hard route, when I've got 2 modules that already work? Particularly when one of those modules was never removed and the other was released, and then removed in favour of this one (which doesn't work). Besides which; I feel my approach is more simple, easier to maintain and more consistent with the other DSC resources? Anyway, have a great Xmas! I'll pick this up when I'm back in the New Year. |
|
Any further thoughts on this? Do I need to fix the existing module if I want this PR in the near-term release? To be honest if so, I may just run my modules in my environment and wait for the major update in April? As I say, I believe the resource type approach is more consistent, easier to maintain etc. Also I don't think this is a new feature problem, I think it is a consequence of the approach taken with the previous module? If my PR will be approved I'll wait, if my PR won't be merged, then I'll reconsider... Ideally though, it'd be included in a near-term release :-D |
|
@dannyKBjj, thanks for submitting this PR and @FabienTschanz thanks for reviewing it. Fabien is correct: We only release breaking changes (like removal of resources or parameters) twice a year: First releases of April or October. In case there is an urgent issue that really breaks something, like the removal of a parameter in a cmdlet that is used, we mark the parameter as Deprecated and leave the parameter. The parameter doesn't work anymore and we write warning messages warning the users that it is deprecated. In the first Breaking change release, the parameter is then removed. Just to summarize:
Just wondering:
@ricmestre, what is your opinion on this PR? |
|
@ykuijs My take is still the same when I said to remove the duplicated resource, it's probably better to have separate resources for this and remove the current one but as you've mentioned that would be a breaking change, so yes either the current one must be accommodated to incorporate the fixes or if the new resources are added then the current must have those warnings added. To be honest in my solution I have the export running automatically so if I update M365DSC I'll get whatever fixes this issue so I don't mind, but for others might not be a solution to have a breaking change outside its schedule. |
|
Ok, then lets move forward with the two resources. Does the Android version of the resource work correctly right now or does it also need updating? @dannyKBjj: Can you please update this PR to make the IntuneAppConfigurationDevicePolicy deprecated instead of removing it. The code should provide warnings that the resource is deprecated and do nothing else. The Readme of the resource should also say it is deprecated. |
|
Hi yes, I can certainly do that. I'm not sure what you mean by: "The code should provide warnings that the resource is deprecated and do nothing else. The Readme of the resource should also say it is deprecated." Where do I put this code, do you have an example I can refer to? Do you mean replace/comment out the code in the old module for Get- Set etc. etc. with a Write-Verbose message to say the module is depracated? Should the message mention the new modules? |
Correct, the code of the Get, Set, Test and Export methods should be replaced with a Write-Verbose message stating that the module is deprecated. The same should be added to the Readme.md of the module. Unfortunately I do not have an example, because we do not have any other resource that is deprecated. |
|
Please also update from Dev. A couple of files have changed |
...sources/MSFT_IntuneAppConfigurationDevicePolicy/MSFT_IntuneAppConfigurationDevicePolicy.psm1
Show resolved
Hide resolved
|
Hi, is anyone reviewing this? |
That is correct. Recently a lot of Intune resources were updated (same issue as described in here). So please make sure the conflicts are corrected. |
|
@dannyKBjj, can you please update with Dev? |
…onDevicePolicy-cannot-handle-'for-Intune'-applications-microsoft#5587
As requested
|
Done. Hopefully all OK this time? |
|
Now unit tests are failing!!? Again, everything is fine at my end. |
|
Same issues with #5640 |
|
I will download your code and run the tests locally to see what is going on. Some background info: What I sometimes do is deleting all modules (except the default ones) from my system or using a freshly installed VM to run the tests. |
|
When I run the tests locally, I get the exact same failing messages. Now checking what the root cause is. |
|
These tests are failing because the resource has been deprecated and the functions always return an empty hashtable. You should update the unit test for IntuneAppConfigurationDevicePolicy to check for an empty array in each function....that is it. The rest of the tests can be removed. |
|
Oh wow, thanks. I'll look into it tomorrow, I could've sworn the errors were in my new modules, but maybe I was hallucinating 😂. Thanks for taking the time to look into it. |
suggested to changes script to script didn't seem to work, but what I've done instead allows it to pass, but leaves original code intact (but commented out). Edits should be obvious. Hopefully this is good enough!
|
tried checking for @{} as returned by the function, that didn't seem to work - strange errors along lines of - expected hashtable, but got hashtable errors. Commented out tests instead and checked for $null where applicable - the script now passes and can easily be reverted should anyone wish to. |
|
hi, all checks passed on this pull a couple of weeks ago, is it possible that someone could review and (hopefully) merge? |
|
hi, still need a review on this please? |
recent changes to Export not reflected in this PR. Updating it so expedite module acceptance.
|
Has issue #5588 been fixed somehow? I was looking at simply fixing the module as perhaps that is an easier merge, but now I am going through my steps to reproduce the issue and it seems to be working properly. I checked the code and the logic looks good. I can't find another pull request related to this though. |
|
It looks to me like I may as well close this PR? |
|
Turns out the issue might've been that you can add the same app twice in the Apps blade. As the original module goes off the name it will pull both bundleIDs. My module didn't have the issue as it goes directly off the bundleID. All in all, it seems like a bug with the Apps blade and not the module, so I'm closing this PR. |
Pull Request (PR) description
MSFT_IntuneMobileAppConfigurationPolicyIOS and MSFT_IntuneAndroidManagedStoreAppConfiguration
were supposed to be pulled in favour of existing DSCResource - IntuneAppConfigurationDevicePolicy as my modules duplicated functionality. Whilst testing on our tenant we discovered the following issue: IntuneAppConfigurationDevicePolicy cannot handle 'for Intune' applications #5587
It seems that for some reason only MSFT_IntuneMobileAppConfigurationPolicyIOS was removed. This pull request adds that module back in and deletes "IntuneAppConfigurationDevicePolicy" as I believe that module is now defunct.
See
#5401
#5587
This Pull Request (PR) fixes the following issues
Task list
Entry should say what was changed and how that affects users (if applicable), and
reference the issue being resolved (if applicable).