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

DRG: New M365DSCDRGUtil.psm1 for reusable DRG functions #3309

Closed
andikrueger opened this issue May 15, 2023 · 19 comments · Fixed by #3452 or #3468
Closed

DRG: New M365DSCDRGUtil.psm1 for reusable DRG functions #3309

andikrueger opened this issue May 15, 2023 · 19 comments · Fixed by #3452 or #3468
Assignees
Labels
Core Engine Enhancement New feature or request

Comments

@andikrueger
Copy link
Collaborator

While browsing the code for some additional information for an issue, I came across the following functions:

https://github.com/microsoft/Microsoft365DSC/blob/Dev/Modules/Microsoft365DSC/DSCResources/MSFT_AADAdministrativeUnit/MSFT_AADAdministrativeUnit.psm1#L1057-L1743

This looks like to be part of the DRG Template. These functions should be removed from all resources and be bundled in a separate module under Modules: M365DSCDRGUtil.psm1

@William-Francillette and @NikCharlebois any thoughts on this?

@NikCharlebois
Copy link
Collaborator

+1

Also, all the ConvertTo- (String, complexobject, etc.) function shouldn't be found a separate functions in each resource, but instead should be defined only once in a central module.

@William-Francillette
Copy link
Contributor

I agree, I developed those functions independently as I didn't want to modify M365DSCUtil and there was a lot of modification since first version
I can create M365DSCDRGUtil.psm1 and include all those functions for new generated resources.
Do you want to update existing resources as well? Each generated resource would need to be tested to make sure they won't break

@andikrueger
Copy link
Collaborator Author

I would create one PR that would contain

  • new module M365DSCDRGUtil.psm1
  • updates to the module manifest of M365DSC
  • changes to all existing resources

We had big PRs in the past, that did not cause any issues.

@William-Francillette
Copy link
Contributor

Sounds like a plan - I'll post that PR along with #3298 changes

@NikCharlebois
Copy link
Collaborator

@William-Francillette any updates on this. Where are we at with centralizing the function and removing it from all resources? Thanks

@William-Francillette
Copy link
Contributor

I was delayed with the setting catalog resource generator which is still not ready.
I'll work on the cleanup separately in the week end and next week and make sure to release the pr during the week

@NikCharlebois
Copy link
Collaborator

Thanks for the update, appreciate your help!

@William-Francillette
Copy link
Contributor

Quick update on this PR:
I finished updating the resources - I will now start the testing phase - Only x72 resource to check 😱

Will post that PR asap

@William-Francillette
Copy link
Contributor

update:
Phase 1 - Unit test via Pester is completed
There was quite more work than expected and fixed some of the resources and unit tests
@NikCharlebois @ykuijs @andikrueger I had to recreate the stubs for MgDirectoryObject used by AADEntitlementManagementConnectedOrganization - Can we include Microsoft.Graph.DirectoryObjects from the dependencies?

Next phase of the test will be the resource export - I'll keep you posted

@andikrueger
Copy link
Collaborator Author

There is a dedicated module within M365DSC to create stubs: https://github.com/microsoft/Microsoft365DSC/blob/Dev/Modules/Microsoft365DSC/Modules/M365DSCStubsUtility.psm1 Looks like, this file misses some of the dependencies...

@William-Francillette
Copy link
Contributor

Thanks @andikrueger - got that one, I will update with that PR

@William-Francillette
Copy link
Contributor

William-Francillette commented Jun 23, 2023

Phase 2A - Export
I was able to export all DRG generated AAD resources successfully
It all looked ok except for:

  1. A small indent issue on AADAuthenticationMethodPolicyAuthenticator: I will need to look a Get-M365DSCDRGComplexTypeToString but this doesn't have an impact on the resource - it's only visual
    image

  2. Trailing characters: I could see some trailing characters (comma and semicolon) from CIM Instances array left by Convert-DSCStringParamToVariable: this should be fixed by Remove trailing commas and semi colons between items of an array of CIM instances ReverseDSC#33

@ricmestre
Copy link
Contributor

@William-Francillette Hi, did you really encounter resources with semicolons? I really thought this wasn't true anymore so my latest iteration on that PR doesn't cover that, only single colons in an whole line. Could you please share one of those examples so I can update the PR?

@William-Francillette
Copy link
Contributor

this on is for the comma
image
semicolon
image
Would be great if you could include all the trailing characters as I am removing the below from all resources
image

@ricmestre
Copy link
Contributor

just updated the PR with this chunk:

  if ($IsCIMArray)
  {
      $DSCBlock = $DSCBlock.Replace("},`r`n", "`}`r`n")
      #$DSCBlock = $DSCBlock.Replace("`r`n,`r`n", "`r`n") # see below
      #$DSCBlock = $DSCBlock.Replace("`r`n;`r`n", "`r`n") # see below
      $DSCBlock = $DSCBlock -replace "`r`n\s*[,;]`r`n", "`r`n" # replace "<crlf>[<whitespace>][,;]<crlf>" with "<crlf>"
  }

William-Francillette added a commit to William-Francillette/Microsoft365DSC-1 that referenced this issue Jun 26, 2023
@William-Francillette
Copy link
Contributor

William-Francillette commented Jul 5, 2023

Quick update (I was off last week):

I have tested all modified AAD resources for compilation successfully

I will be posting a PR in ReverseDSC for trailing character soon

I have started testing update for those resources but haven't completed yet as I found some issues

Hope posting this PR this week

@William-Francillette
Copy link
Contributor

AAD resources test completed 🥳

William-Francillette added a commit to William-Francillette/Microsoft365DSC-1 that referenced this issue Jul 6, 2023
@William-Francillette
Copy link
Contributor

ok just finished to merge with latest V2.0 update - I will need to retest everything as I had to resolve many conflict manually

I hope pushing this PR in the week end ( I was nearly done lol 🥲)

@ricmestre
Copy link
Contributor

@William-Francillette Really looking forward for this, great work!

I know it's not related but didn't want to create a new issue just for it, would it be possible to update the DRG to create new types of instances for Assignments as @NikCharlebois requested, e.g. MSFT_IntuneWindowsInformationProtectionPolicyWindows10MdmEnrolledAssignments instead of being hardcoded to MSFT_DeviceManagementConfigurationPolicyAssignments?

I'm asking about this because it'd really ease the work on my side because on a next step I'd like to have that object extended to accept also DisplayName and not only GroupID, right now I have my code adapted to connect to Graph in order to translate GroupId to DisplayName and vice-versa.

William-Francillette added a commit to William-Francillette/Microsoft365DSC-1 that referenced this issue Jul 9, 2023
William-Francillette added a commit to William-Francillette/Microsoft365DSC-1 that referenced this issue Jul 10, 2023
William-Francillette added a commit to William-Francillette/Microsoft365DSC-1 that referenced this issue Jul 11, 2023
NikCharlebois added a commit to William-Francillette/Microsoft365DSC-1 that referenced this issue Jul 11, 2023
NikCharlebois added a commit that referenced this issue Jul 11, 2023
PR: M365DSCDRGUtil module + Resource Clean-up - Fixes #3309
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Core Engine Enhancement New feature or request
Projects
None yet
4 participants