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

AADServicePrincipal - fix AppId assignment #5605

Merged
merged 4 commits into from
Jan 17, 2025

Conversation

niwamo
Copy link
Contributor

@niwamo niwamo commented Jan 7, 2025

Pull Request (PR) description

Fixes the assignment of the AppId for the AADServicePrincipal resource. It's currently being set to $appInstance.DisplayName, which does not get instantiated at all in most cases. Even in those cases where it does, DisplayName is the wrong property. The AADServicePrincipal AppId property exists on the AADServicePrincipal object, which is always defined.

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. (NOT APPLICABLE)
  • Resource documentation added/updated in README.md. (NOT APPLICABLE)
  • Resource settings.json file contains all required permissions. (NOT APPLICABLE)
  • Examples appropriately added/updated. (NOT APPLICABLE)
  • Unit tests added/updated. (NOT APPLICABLE)
  • New/changed code adheres to DSC Community Style Guidelines.

@niwamo niwamo requested a review from NikCharlebois as a code owner January 7, 2025 01:32
@niwamo
Copy link
Contributor Author

niwamo commented Jan 7, 2025

@niwamo please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@microsoft-github-policy-service agree [company="{your company}"]

Options:

  • (default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
@microsoft-github-policy-service agree
  • (when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.
@microsoft-github-policy-service agree company="Microsoft"

Contributor License Agreement

@microsoft-github-policy-service agree

@niwamo
Copy link
Contributor Author

niwamo commented Jan 9, 2025

My mistake for not checking the unit tests the first time. The failing test has been patched.

It seems clear that the AADServicePrincipal's AppId property should be equal to the AADServicePrincipal's AppId (rather than its DisplayName). Unsure why the test was configured this way.

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.

Just a small comment on the update to the changelog

CHANGELOG.md Outdated Show resolved Hide resolved
Unit test expected AppId to be equal to DisplayName instead of AppId
@niwamo niwamo force-pushed the Fix_AADServicePrincipal branch from 58df889 to c9bcef9 Compare January 15, 2025 17:53
@niwamo
Copy link
Contributor Author

niwamo commented Jan 16, 2025

@ykuijs @NikCharlebois This is a two-line fix for an issue that breaks the generated M365TenantConfig.ps1 file. Can we get this merged (soon)?

@ykuijs
Copy link
Member

ykuijs commented Jan 17, 2025

Just started the unit tests......if that succeeds successfully, we will merge the PR so it will be included in next weeks release.

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 ykuijs merged commit a59a6d2 into microsoft:Dev Jan 17, 2025
2 checks passed
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.

AADServicePrincipal: AppId is missing from configuration files and cannot be compiled
2 participants