-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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 guidance for types which should not have new structural properties #537
Conversation
@microsoft/graphguidelinesapprovers would someone else like to take a look at this guidance? |
Co-authored-by: Garrett DeBruin <[email protected]>
Co-authored-by: Garrett DeBruin <[email protected]>
Co-authored-by: Garrett DeBruin <[email protected]>
Co-authored-by: Garrett DeBruin <[email protected]>
Pinging @microsoft/graphguidelinesapprovers for a review |
@@ -332,6 +333,15 @@ For a complete mapping of error codes to HTTP statuses, see | |||
|
|||
<a name="api-contract-and-non-backward-compatible-changes"></a> | |||
|
|||
### Limitations on core types |
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.
Can we add this guidance for applications and servicePrincipals too? In my mind those are core types.
We actually now have a very real example of why this is important for downstream tooling. Adding a structural property that is handled by a different workload (from the core workload) is really difficult to model with Bicep. If instead it's added as a new entity + nav property, this fits cleanly with Bicep (and ARM's) nested child resources.
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.
Adding a structural property that is handled by a different workload (from the core workload)
FYI, I think we have a different ADR that says we should stop using composite entities.
I'm ok adding applications
and servicePrincipals
, but I don't think those have been brought up with the API council. Do we want to take this PR as-is and do a follow-up to add apps and sps after discussion with the council?
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.
I am inclined to take Garrett's suggestion. If you are fine with this, I will merge as-is @dkershaw10
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.
Looks good, but added some suggestions. You might want to discuss with other folks whether apps and SPs should be classed as core types (IMHO they are, but you should check).
Fix grammar
Use proper noun name for graph product
Rename `bankAccountInformation` to `bankAccountDetail`
Fix grammar
Add link to nav prop article
Update to `existing core types`
Update description of core types in overview to addd "intrinsic"
Adding guidance that there should not be new structural properties added to
user
,group
,device
.