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

PRIORITY / URGENT - Migrate MSCloudLoginAssistant authentication context access to cmdlets #5540

Merged

Conversation

FabienTschanz
Copy link
Contributor

Pull Request (PR) description

This PR migrates the $global:MSCloudLoginConnectionProfile access to its specific workload cmdlet counterpart that was introduced with version 1.1.29 of MSCloudLoginAssistant. Without this change, every resource that uses any part of the authentication context will fail!

This Pull Request (PR) fixes the following issues

None.

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.

@ykuijs
Copy link
Member

ykuijs commented Dec 11, 2024

Please add the changelog entry under the 1.24.1211.1 version....we will include it in this weeks release

@FabienTschanz
Copy link
Contributor Author

Alright, will do.

@FabienTschanz FabienTschanz force-pushed the feat/migrate-mscloudlogin-auth-context branch from 6dafa4e to 28ee71b Compare December 11, 2024 21:03
@FabienTschanz
Copy link
Contributor Author

@ykuijs And done. Ready for review.

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
Copy link
Member

ykuijs commented Dec 11, 2024

Unit tests are failing because the Get-MSCloudLoginConnectionProfile function is not mocked and therefore unknown. Can you please add that to all unit test of impacted modules?

@FabienTschanz
Copy link
Contributor Author

On my way.

@FabienTschanz
Copy link
Contributor Author

Let's see how this one goes. I also added the two new commands to the unit tests template for the resource generator.

@FabienTschanz
Copy link
Contributor Author

Still not quite. I'm working on it.

@ykuijs
Copy link
Member

ykuijs commented Dec 11, 2024

Nope, not yet. You can't mock functions that haven't been declared yet.

To solve it, add the function declaration (no function body required) with the same parameters to the file Tests\Unit\Stubs\Generic.psm1

@FabienTschanz
Copy link
Contributor Author

Phew, that was a bunch of stuff to think about... Should have gotten it on the first try though.

@ykuijs
Copy link
Member

ykuijs commented Dec 11, 2024

That should do it. Thanks for implementing this so fast, really appreciated!

@FabienTschanz
Copy link
Contributor Author

That should do it. Thanks for implementing this so fast, really appreciated!

Sure thing. Hope the tests work (at least the couple ones I checked now did succeed on my machine). I missed a Write-Verbose -Message $PSScriptRoot -Verbose at the beginning of the MSCloudLoginAssistant.psm1 file, so we need to remove that in the future. I will also open a PR there that enables to disconnect the Microsoft Graph workload and also only disconnect a specific workload. But that's something for the future (maybe tomorrow if I'm bored 😆).

@ykuijs
Copy link
Member

ykuijs commented Dec 11, 2024

Nik already ran into that Verbose message when testing an export. Definitely something we need to fix, but not a P1 issue for now

@ykuijs
Copy link
Member

ykuijs commented Dec 11, 2024

Worked like a charm. Merging now!

@ykuijs ykuijs merged commit 936ec1a into microsoft:Dev Dec 11, 2024
2 checks passed
@FabienTschanz FabienTschanz deleted the feat/migrate-mscloudlogin-auth-context branch December 11, 2024 22:21
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.

2 participants