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

Update workload-identity-federation-config-app-trust-managed-identity.md #1322

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

EvanSchallerer
Copy link

@EvanSchallerer EvanSchallerer commented Jan 16, 2025

Add note about the Microsoft Graph Bicep feature limitations that apply to the bicep sample.
Also disambiguates the example GUID used for the application id from that used for the subject in order to avoid confusion.

Add note about the Microsoft Graph Bicep feature limitations that apply to the bicep sample.
Copy link
Contributor

@EvanSchallerer : Thanks for your contribution! The author(s) have been notified to review your proposed change.

Copy link
Contributor

Learn Build status updates of commit 55d1475:

⚠️ Validation status: warnings

File Status Preview URL Details
docs/workload-id/workload-identity-federation-config-app-trust-managed-identity.md ⚠️Warning Details

docs/workload-id/workload-identity-federation-config-app-trust-managed-identity.md

  • Line 139, Column 170: [Warning: hard-coded-locale - See documentation] Link 'https://learn.microsoft.com/en-us/graph/templates/limitations#unsupported-deployment-features' contains locale code 'en-us'. For localizability, remove 'en-us' from links to most Microsoft sites.
  • Line 139, Column 170: [Suggestion: docs-link-absolute - See documentation] Absolute link 'https://learn.microsoft.com/en-us/graph/templates/limitations#unsupported-deployment-features' will be broken in isolated environments. Replace with a relative link.

For more details, please refer to the build report.

Note: Your PR may contain errors or warnings or suggestions unrelated to the files you changed. This happens when external dependencies like GitHub alias, Microsoft alias, cross repo links are updated. Please use these instructions to resolve them.

For any questions, please:

Copy link
Contributor

Learn Build status updates of commit 825a787:

💡 Validation status: suggestions

File Status Preview URL Details
docs/workload-id/workload-identity-federation-config-app-trust-managed-identity.md 💡Suggestion Details

docs/workload-id/workload-identity-federation-config-app-trust-managed-identity.md

  • Line 139, Column 170: [Suggestion: docs-link-absolute - See documentation] Absolute link 'https://learn.microsoft.com/graph/templates/limitations#unsupported-deployment-features' will be broken in isolated environments. Replace with a relative link.

For more details, please refer to the build report.

Note: Your PR may contain errors or warnings or suggestions unrelated to the files you changed. This happens when external dependencies like GitHub alias, Microsoft alias, cross repo links are updated. Please use these instructions to resolve them.

For any questions, please:

Disambiguate example application ID guid to avoid confusion with the managed identity guid.
Copy link
Contributor

Learn Build status updates of commit 255ec05:

💡 Validation status: suggestions

File Status Preview URL Details
docs/workload-id/workload-identity-federation-config-app-trust-managed-identity.md 💡Suggestion Details

docs/workload-id/workload-identity-federation-config-app-trust-managed-identity.md

  • Line 139, Column 170: [Suggestion: docs-link-absolute - See documentation] Absolute link 'https://learn.microsoft.com/graph/templates/limitations#unsupported-deployment-features' will be broken in isolated environments. Replace with a relative link.

For more details, please refer to the build report.

Note: Your PR may contain errors or warnings or suggestions unrelated to the files you changed. This happens when external dependencies like GitHub alias, Microsoft alias, cross repo links are updated. Please use these instructions to resolve them.

For any questions, please:

@Jak-MS
Copy link
Contributor

Jak-MS commented Jan 16, 2025

@cilwerner
Can you review the proposed changes?

Important: When the changes are ready for publication, adding a #sign-off comment is the best way to signal that the PR is ready for the review team to merge.

#label:"aq-pr-triaged"
@MicrosoftDocs/public-repo-pr-review-team

Copy link
Contributor

@cilwerner cilwerner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes are needed here before merging

@@ -101,7 +101,7 @@ In this section, you'll configure a federated identity credential on an existing
Open a terminal in your preferred IDE and run the following command to create a federated identity credential on your app. Replace the GUID with the Object (principal) ID of the managed identity.

```CLI
az ad app federated-credential create --id 00001111-aaaa-2222-bbbb-3333cccc4444 --parameters credential.json
az ad app federated-credential create --id 11112222-bbbb-3333-cccc-4444dddd5555 --parameters credential.json
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not be changed, this sample GUID is mentioned again in L113. Please revert.

Suggested change
az ad app federated-credential create --id 11112222-bbbb-3333-cccc-4444dddd5555 --parameters credential.json
az ad app federated-credential create --id 00001111-aaaa-2222-bbbb-3333cccc4444 --parameters credential.json

Copy link
Author

@EvanSchallerer EvanSchallerer Jan 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is the reason I changed it, I believe the GUID on line 113 should be different. It's my understanding that the Object ID of the managed identity is different than the application ID which becomes confusing when this doc shows the same sample guid for both.

…-managed-identity.md


Change link to MS Graph limitations doc to be relative per PR review suggestion.

Co-authored-by: Chris Werner <[email protected]>
Copy link
Contributor

Learn Build status updates of commit f8a49b8:

✅ Validation status: passed

File Status Preview URL Details
docs/workload-id/workload-identity-federation-config-app-trust-managed-identity.md ✅Succeeded

For more details, please refer to the build report.

For any questions, please:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants