-
Notifications
You must be signed in to change notification settings - Fork 33
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
Test ARM, test case for location-based resource #703
Conversation
packages/cadl-ranch-specs/http/azure/resource-manager/models/resources/location.tsp
Outdated
Show resolved
Hide resolved
packages/cadl-ranch-specs/http/azure/resource-manager/models/resources/location.tsp
Outdated
Show resolved
Hide resolved
packages/cadl-ranch-specs/http/azure/resource-manager/models/resources/location.tsp
Outdated
Show resolved
Hide resolved
packages/cadl-ranch-specs/http/azure/resource-manager/models/resources/location.tsp
Outdated
Show resolved
Hide resolved
packages/cadl-ranch-specs/http/azure/resource-manager/models/resources/location.tsp
Outdated
Show resolved
Hide resolved
packages/cadl-ranch-specs/http/azure/resource-manager/models/resources/location.tsp
Outdated
Show resolved
Hide resolved
overall lgtm. @XiaofeiCao please help to ensure the mock api is right before merge. |
@@ -907,6 +907,162 @@ Expected response body: | |||
} | |||
``` | |||
|
|||
### Azure_ResourceManager_Models_Resources_LocationResources_createOrUpdate |
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 prefer to call this scenario Azure_ResourceManager_Models_Resources_Location_ResourceGroup_createOrUpdate
or
Azure_ResourceManager_Models_Resources_Location_ResourceGroupLocationResource_createOrUpdate
,
since this PR covers ResourceGroupLocationResource
, and we also have SubscriptionLocationResource
and TenantLocationResource
which we don't cover here.
@weidongxu-microsoft @tadelesh Would like your opinion.
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 prefer the shorter one.
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.
@iscai-msft Here's another case where we may need @clientName
:
We want the scenario name be Azure_ResourceManager_Models_Resources_Location_ResourceGroup_createOrUpdate
, this may require an interface called ResourceGroup
.
For generated SDK, it'll generate a client called ResourceGroup
... This is not a big issue since it resides under the corresponding namespace, though still a bit weird to me...
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.
@XiaofeiCao is it possible for you to compile a list of all of the categories and subcategories, so we can agree on the current ordering? Everything's getting long and confusing imo. Something like
Azure
|-> ResourceManager
|-> Models
|-> Resources
| -> CommonTypes
For example, I don't think we need Models
as a namespace
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.
@iscai-msft Here's what's intended to be:
Azure
|-> ResourceManager
|-> Models
|-> Resources // resources and their common CRUD operations
|-> CommonTypes // special common-types like ManagedIdentity
|-> ManagedIdentities
|-> EncryptionProperties
|-> SKUs, etc
|-> Operations // uncommon operations, or operations with multiple scenarios, irrelavant of resource types
|-> LROs(including pageable LROs)
|-> Resource Actions //import, export, upload, trigger, etc
|-> Resource Move // move resource to another subscription(not sure, maybe just in Resources is enough?)
Basically it's following this issue's structure, and typespec-azure
's sample folder. Any suggestions are welcome too!
Models
seems unnecessary, we could remove that.
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.
Here is my suggestion, @timotheeguerin please lmk your thoughts
Azure
|-> ResourceManager
|-> Resources // resources and their common CRUD operations
|-> Actions
|-> Move (?)
|-> CommonTypes // special common-types like ManagedIdentity
|-> ManagedIdentities
|-> EncryptionProperties
|-> SKUs, etc
|-> Operations
|-> LROs(including pageable LROs)
The previously referenced ARM Resourcec type |
Cadl Ranch Contribution Checklist:
@scenario
names. Someone can look at the list of scenarios and understand what I'm covering.@scenarioDoc
s for extra scenario description and to tell people how to pass my mock api check.