-
Notifications
You must be signed in to change notification settings - Fork 520
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
IntuneDeviceConfigurationPolicyWindows10: Support setting assignment groups by display name #3763
Merged
Merged
Changes from 9 commits
Commits
Show all changes
19 commits
Select commit
Hold shift + click to select a range
4f0fb5e
Support setting assignment groups by display name
ricmestre 5142a3e
No intention to change this to lowercase
ricmestre efe6f69
Fix example file
ricmestre 6b43ef3
Change property to odataType and remove groupId if unique
ricmestre 6e1de6c
Missed in previous
ricmestre 0d3028a
Merge branch 'Dev' of https://github.com/microsoft/Microsoft365DSC in…
ricmestre 844ea40
Change property name back to dataType
ricmestre 55dd571
Merge branch 'Dev' of https://github.com/microsoft/Microsoft365DSC in…
ricmestre 4291932
Merge branch 'Dev' into Dev
NikCharlebois 24fe7db
Merge branch 'Dev' of https://github.com/microsoft/Microsoft365DSC in…
ricmestre b8b267e
Merge branch 'Dev' of github.com:ricmestre/Microsoft365DSC into Dev
ricmestre 67b6fcb
Add groupDisplayName to all MSFT_DeviceManagementConfigurationPolicyA…
ricmestre 59c5d64
Missed in previous
ricmestre fa16720
Change CIMInstance name back to MSFT_DeviceManagementConfigurationPol…
ricmestre c6b7091
Remove duplicated property
ricmestre 3c3d24a
Merge branch 'Dev' of https://github.com/microsoft/Microsoft365DSC in…
ricmestre 89a4474
Missed to change CIMInstance name in schema
ricmestre 51453f3
Merge branch 'Dev' of https://github.com/microsoft/Microsoft365DSC in…
ricmestre bfd2ae2
Fix CIM instance type
ricmestre File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would result in a breaking change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this change really needed? The CIM Instance is embedded into this resource. Are there any other CIM Instances with the previous name somewhere else (and do they have the same properties?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically almost all Intune resources that support Assignments use exactly the same CIM instance MSFT_DeviceManagementConfigurationPolicyAssignments, which means that in order to support the changes I made in this PR without changing the CIM instance name I would need to change all Intune resources at the same time otherwise it gives error messages when importing the module.
I discussed this privately with Nik several months ago.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do all other resources need the display name parameter? If yes, we should change it globally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am fine with this PR starting by addressing one resource, and then later submit a larger PR to address them all. However, we need to confirm whether or not we really need to rename the CIMInstance right now for this to work or if this can be done in April. I can hold today's release for a few hours, but if we can't resolve the renaming question soon, this will need to get pushed to next week at a minimum. Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. What I am suggesting for now is to keep the additional code in, but keep the current CIMInstance name. Then later submit two other PRs. The first one to add the data type to all resources, which could get released anytime when ready, then a second one to rename the CIMInstance which would get release as breaking changes in April. Would that be feasible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
groupDisplayName, not dataType :) But yeah that's what I mentioned keep the code I added, change CIMInstance name back to the original but then I'll need to add groupDisplayName property to all resources in this same PR otherwise it won't work, all resources must be changed at the same time, then the code to make them work could be added later on.
That being said, do you still prefer to have individual CIMInstance names per resource (making those changes in April) or keep the same name for all of them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coming from a standpoint, that we should reuse as much code as possible, and if there is no eminent reason to change the CIM Instance name, I would stick one name across all resources.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I was explaining the current situation to a colleague, I realized that what I am suggesting above doesn't make sense. In doing so, we would end up having two sets of definition for the same CIMInstance namespace. One where datatype is present and one where it isn't. Off course the fix would be to make sure datatype is added to all MSFT_DeviceManagementConfigurationPolicyAssignments schemas. Sorry for not picking up on this earlier. I will go ahead and release 1.23.1011.1 without this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In latest iteration all, well almost all, Intune resources have now an additional property called groupDisplayName inside MSFT_DeviceManagementConfigurationPolicyAssignments.