add armResourceType scalar in typespec-azure-core
#1272
Conversation
ArcturusZhang
requested review from
bterlson,
markcowl,
allenjzhang and
timotheeguerin
as code owners
|
All changed packages have been documented.
Show changes
|
|
You can try these changes here
|
ArcturusZhang
force-pushed
the
add-resource-type
branch
from
fa8e876
to
ac93669
Compare
|
@allenjzhang could you help to review? |
Yeah I assume we should go through design discussions before we could merge this. What is the process of that? |
|
We can talk about it tomorrow in dpg meeting(or later one if we can't) don't think this needs to be brought up to the Wednesday one |
|
@timotheeguerin any update on this? |
| * Microsoft.Network/virtualNetworks/subnets | ||
| * ``` | ||
| */ | ||
| scalar resourceType extends string; |
There was a problem hiding this comment.
Sorry, I was OOF the last 2 weeks, has there been agreement we needed this and this was the name we wanted?
There was a problem hiding this comment.
Oh - I do not know. Do we need to bring up this topic in one of our typespec sync up meeting or your internal typespec sync up meeting?
In those meetings that I have participated in, this has not been a topic there.
Or maybe we could put this as an agenda in our next typespec sync up meeting on your wednesday night.
There was a problem hiding this comment.
yeah would be good, do you need this for this release?
There was a problem hiding this comment.
I think we probably need to differentiate this as we did with armResourceIdentifier - armResourceType.
There is definitely use for this in ARM, and since the type is needed to properly specify the allowedResourceType in armResourceIdentifier, it seems appropriate to put this here.
There was a problem hiding this comment.
Yeah I agree the name should be armResourceType which makes more sense.
The second part is that you suggest we add a generic argument for allowed resource type? like this?
scalar armResourceType<AllowedResourceTypes extends ArmResourceIdentifierAllowedResource[] = never> extends string {}
I think this looks weird because this type is modeling the resource type of myself, if there is a limited list of resource types that this field is supporting, should we define a extensible enum based on resource type like this?
union SupportedResourceType extends armResourceType {
"Microsoft.Compute/virtualMachines"
}
Well but I could see values for this because we could update the definition of Resource and TrackedResource to let them accept another argument of resource type like this:
model TrackedResource<Properties extends Model, ResourceType extends ArmResourceIdentifierAllowedResource> {
id: armResourceIdentifier;
type: armResourceType<ResourceType>;
properties: Properties;
}
so that we finally have explicitly defined resource type on resources, currently arm resources could have resource types but they are implicitly - we can infer them from the uri paths we built for resource related operations.
There was a problem hiding this comment.
yeah I think using a union here instead to be more precise makes more sense instead of the armREsourceIdentifier which does that for a subset of the value.
and +1 on the name being armResourceType
Co-authored-by: Timothee Guerin <[email protected]>
ArcturusZhang
changed the title
typespec-azure-corearmResourceType scalar in typespec-azure-core
| @@ -86,6 +86,7 @@ function cleanupDocument(original: OpenAPI2Document): OpenAPI2Document { | |||
|
|
|||
| replaceUuidRefs(document, "Azure.Core.uuid"); | |||
| replaceUuidRefs(document, "Azure.Core.azureLocation"); | |||
| replaceUuidRefs(document, "Azure.Core.armResourceType"); | |||
There was a problem hiding this comment.
What does this actually do? It seems like this should have some impact on a generated common-types swagger, but I don't see it in this PR
There was a problem hiding this comment.
Same comment as you, Mark. Why is this used? Why did we add this line in this scenario? We should have changes in types.json , but they are not present in this pull request.
markcowl
added
the
needs-info
Fixes #1268