-
Notifications
You must be signed in to change notification settings - Fork 41
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
Adding a legacy Required properties resource mix models #833
Conversation
❌ There is undocummented changes. Run The following packages have changes but are not documented.
Show changes |
…zhang_addRequiredProperties
|
||
namespace Azure.ResourceManager.Legacy; | ||
|
||
//#region Standard Resource Operation Interfaces |
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 we supprot region in tsp?
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.
remove
@doc("Concrete tracked resource types can be created by aliasing this type using a specific property type.") | ||
@armResourceInternal(Properties) | ||
@includeInapplicableMetadataInPayload(false) | ||
model TrackedResourceWithRequiredProperties<Properties extends {}> |
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.
Should we put #deprecated
on all of those or have a linting rule against using them?
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 feel the namespace being legacy which has good separation. It is targeted for brownfield. Unless ARM has disallowed required properties (currently they don't) we probably don't want to put deprecate.
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 think we should consider a different approach, since this leaves out many important brownfield scenarios. Also, a couple of inline nits
@@ -0,0 +1,59 @@ | |||
using TypeSpec.Http; |
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.
For legacy, I think we are still leaving out scenarios where folks extend Resource or EntityResource. I would prefer it if we just let them extend a common-types resource type, and added a new decorator they would need to decorate the type with that is the equivalent of @armResourceInternal
and @includeInapplicableMetadataInPayload
, and relax the rules around extension to include this case.
|
||
namespace Azure.ResourceManager.Legacy; | ||
|
||
//#region Standard Resource Operation Interfaces |
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.
remove
@@ -40,7 +40,7 @@ model ResourceNameParameter< | |||
@armResourceInternal(Properties) | |||
@includeInapplicableMetadataInPayload(false) | |||
model TrackedResource<Properties extends {}> extends Foundations.TrackedResource { | |||
@doc("The resource-specific properties for this resource.") | |||
@doc("The resource-specific optional properties for this resource.") |
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 don't think these doc updates are necessary. The typespec and the swagger already say it is optional, adding the documentation makes swagger changes without adding any information.
This PR is ON HOLD until the discussion of @visibility design issues.. |
Putting in a separate namespace avoid name conflict.
#131