-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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
Add properties of Identity. #8647
Conversation
No commit pushedDate could be found for PR 8647 in repo Azure/azure-rest-api-specs |
Azure Pipelines successfully started running 1 pipeline(s). |
azure-sdk-for-go - Release
|
azure-sdk-for-js - Release
|
azure-sdk-for-java - Release
|
azure-sdk-for-net - Release
|
azure-sdk-for-python - Release
- Breaking Change detected in SDK
|
Can one of the admins verify this patch? |
@@ -1606,16 +1606,39 @@ | |||
"type": "string", | |||
"description": "The identity type.", | |||
"enum": [ | |||
"SystemAssigned" | |||
"SystemAssigned", |
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.
will this cause breaking changes
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 shouldn't be breaking. It's adding new value to enum and added a not required property. Old schema should still be accepted.
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 you have SDK released for this? As the enum is not modeled as string, new SDK released will be broken because the values in enum could be different
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
"type": "string", | ||
"description": "The principal id of user assigned identity." | ||
}, | ||
"tenantId": { |
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.
The public docs and the MSI spec seem to imply that the contract has changed to "principalId" and "clientId"... However, the contracts in ARM still have these properties. Which is correct? https://microsoft.sharepoint.com/:w:/r/teams/azureresourcemanagerteam/_layouts/15/Doc.aspx?sourcedoc=%7B66505FCE-8057-4524-B84F-4CEFE2D8A77C%7D&file=ARMResourcesWithStrongIdentity.docx&action=default&mobileredirect=true
I've seen more RPs using clientId recently
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 is inline with current service behavior, we are trying to make them sync for now.
I talked to the code owner of the the backend service, will submit a new api version and swagger when he makes the 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.
ok, please mark them as readOnly since the user is not providing these values
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.
Approved with 1 outstanding comment (adding readonly to identity attributes)
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
azure-cli-extensions - Release
No readme.md specification configuration files were found that are associated with the files modified in this pull request, or swagger_to_sdk section in readme.md is not configured
|
@dandanwang0320 , the tool check detected the breaking change in this PR, could you provide the business justification about this breaking change and what the impact of that? |
Azure Pipelines successfully started running 1 pipeline(s). |
* Add missed properties of Identity. * Fix prettier check. * Add read only according to comment.
Latest improvements:
MSFT employees can try out our new experience at OpenAPI Hub - one location for using our validation tools and finding your workflow.
Contribution checklist:
ARM API Review Checklist
Failure to comply may result in delays for manifest application. Note this does not apply to data plane APIs.
Please follow the link to find more details on API review process.