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

Fix for issue - #5587 #5588

Open
wants to merge 2 commits into
base: Dev
Choose a base branch
from

Conversation

dannyKBjj
Copy link
Contributor

@dannyKBjj dannyKBjj commented Dec 19, 2024

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

  • Added an entry to the change log under the Unreleased section of the file CHANGELOG.md.
    Entry should say what was changed and how that affects users (if applicable), and
    reference the issue being resolved (if applicable).
  • Resource parameter descriptions added/updated in the schema.mof.
  • Resource documentation added/updated in README.md.
  • Resource settings.json file contains all required permissions.
  • Examples appropriately added/updated.
  • Unit tests added/updated.
  • New/changed code adheres to DSC Community Style Guidelines.

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.
@FabienTschanz
Copy link
Contributor

@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 IntuneAppConfigurationDevicePoliy instead of replacing it with new resources.

@dannyKBjj
Copy link
Contributor Author

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).

@FabienTschanz
Copy link
Contributor

@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.

@dannyKBjj
Copy link
Contributor Author

dannyKBjj commented Dec 24, 2024

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:

image

It then uses value this to retrieve the 'Id' in Set-Target (see below)
image

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.

@dannyKBjj
Copy link
Contributor Author

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

@ykuijs
Copy link
Member

ykuijs commented Jan 14, 2025

@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:

  • The IntuneMobileAppConfigurationPolicyIOS and IntuneAndroidManagedStoreAppConfiguration resource were added a while back. IntuneMobileAppConfigurationPolicyIOS was removed again, but the IntuneAndroidManagedStoreAppConfiguration resource wasn't (my bad). This was because IntuneAppConfigurationDevicePolicy was already able to configure both and therefore both resource were duplicating functionality. This has been discussed in IntuneMobileAppConfigurationPolicyIOS is not needed #5459.
  • Now that resource has a different issue. And instead of fixing it, you want to reintroduce the two resources and remove the generic one.
    Am I correct?

Just wondering:

  • Why isn't fixing the IntuneAppConfigurationDevicePolicy resource possible? If the current logic is incorrect, can be simplified or needs improvement, this would be the prefered option.
  • If we indeed decide to move forward, we should not delete the old resources but instead make them deprecated and output warnings. Get and Set methods should output warnings and do nothing. Test method should output warning and always return True.

@ricmestre, what is your opinion on this PR?

@ricmestre
Copy link
Contributor

@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.

@ykuijs
Copy link
Member

ykuijs commented Jan 14, 2025

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.

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.

IntuneAppConfigurationDevicePolicy cannot handle 'for Intune' applications
4 participants