[EngSys] Add sample "widget" using folder structure v2 #34823
Conversation
Next Steps to Merge✅ All automated merging requirements have been met! To get your PR merged, see aka.ms/azsdk/specreview/merge. |
|
PR validation pipeline restarted successfully. If there is ApiView generated, it will be updated in this comment. |
API Change CheckAPIView identified API level changes in this PR and created the following API reviews
|
AzureRestAPISpecReview
added
BreakingChangeReviewRequired
| @@ -33,7 +33,7 @@ These settings apply only when `--tag=package-2021-11-01` is specified on the co | |||
|
|
|||
| ```yaml $(tag) == 'package-2021-11-01' | |||
| input-file: | |||
| - Microsoft.Contoso/stable/2021-11-01/contoso.json | |||
| - WidgetManager/stable/2021-11-01/contoso.json | |||
There was a problem hiding this comment.
If all the swaggers have been moved into sub folder, then we probably should not have the namespace level readme.md file here.
There was a problem hiding this comment.
I think the namespace should keep as Microsoft.Contoso?
There was a problem hiding this comment.
Contoso.Widget is the typespec v1 folder structure convention. I think.
There was a problem hiding this comment.
@JeffreyRichter recommended exactly this folder structure:
The are 2 Contoso services: WidgetManagement (control-plane) and Widget (data-plane)
The RP Namespace is Contoso.WidgetUnder data-plane, the folder should be "Widget"
Under resource-manager, the immediate folder is the RP Namespace: "Contoso.Widget"
Under this "Contoso.Widget" folder is the control-plane service name folder: "WidgetManagement"
There was a problem hiding this comment.
If all the swaggers have been moved into sub folder, then we probably should not have the namespace level readme.md file here.
The "folder structure v2" word document shows two readmes, one in the RP namespace folder, and one in the service folder.
There was a problem hiding this comment.
I think this logic makes sense. Just have one more point to confirm, this is only for swagger folder structure right? And we do allow to have typespec folder structure as v2 but swagger folder structure as v1. right?
To make sure I'm answering the correct question, can you provide an example folder structure, you want to know is valid?
There was a problem hiding this comment.
This is for TypeSpec conversion, I know that Matthew is okay to fold the new folder structure into wave 2 conversion but I think what he mean is to adopt to v2 for the converted TypeSpec, and I don't think the conversion should also be responsible for existing v1 swagger folder structure migrate to v2? Unless I miss understood.
There was a problem hiding this comment.
- Correct, All Wave 2 TypeSpec conversions SHOULD use the v2 folder structure.
- Service teams splitting swagger services HAVE TO use the v2 folder structure.
- Any service teams that make new PRs CAN also use it and I'd like this, but we don't have to fight hard to make it happen.
- Any existing services with no new PRs can stay as is.
There was a problem hiding this comment.
This is for TypeSpec conversion, I know that Matthew is okay to fold the new folder structure into wave 2 conversion but I think what he mean is to adopt to v2 for the converted TypeSpec, and I don't think the conversion should also be responsible for existing v1 swagger folder structure migrate to v2? Unless I miss understood.
I do not understand what you mean by "adopt to v2 for the converted TypeSpec, [...] but not be responsible for existing v1 swagger folder structure migrate to v2". Please state the exact folder structure you think should be allowed, and why. Use an existing v1 spec as an example, like contosowidgetmanager.
There was a problem hiding this comment.
I was referring to changes like this https://github.com/Azure/azure-rest-api-specs/pull/34549/files# which was created based on a tsp conversion pr.
If we don't want the tsp conversion to take the work of migrating existing swagger folder structure v1 to new folder structure v2, but we want the converted typespec to use folder structure v2 directly, this #34549 is what we get tsp v2 mixed with swagger v1.
If we want the tsp conversion also take care of the existing swagger folder structure, then everything will be v2. But I am not sure if we should mix the migration work with the folder structure work?
specification/contosowidgetmanager/resource-manager/Contoso.Widget/readme.md
Outdated
Show resolved
Hide resolved
mikeharder
changed the title
specification/widget/resource-manager/Microsoft.Widget/readme.md
Outdated
Show resolved
Hide resolved
specification/widget/resource-manager/Microsoft.Widget/WidgetManagement/tspconfig.yaml
Outdated
Show resolved
Hide resolved
…anagement/tspconfig.yaml Co-authored-by: ZiWei Chen <[email protected]>
…anagement/tspconfig.yaml Co-authored-by: ZiWei Chen <[email protected]>
| /** Microsoft.Contoso Resource Provider management API. */ | ||
| @armProviderNamespace | ||
| @service(#{ title: "WidgetManagement" }) | ||
| @versioned(WidgetManagement.Versions) | ||
| namespace WidgetManagement; |
There was a problem hiding this comment.
despite this is a demo, we should ensure the typespec is correct.
Here we should either change the namespace here to its provider namespace (Microsoft.Widget, the same as we have in the filepath), or add an argument to the armProviderNamespace decorator
There was a problem hiding this comment.
I agree we want this sample to be as correct as possible, including the TSP itself, and tspconfig.yaml. However, this raises the point, we may need to validate the TSP namespace matches the service name and/or rp namespace.
Typically speaking for a public RP, this makes sense that there would only be 1 RP namespace per organization. However, considering RPaaS supports multiple Private RPs per org for testing purposes (https://armwiki.azurewebsites.net/rpaas/prp-onboarding.html), there might be cases (such as ours - https://github.com/Azure/azure-rest-api-specs-pr/tree/RPSaaSDev/specification/healthcareinterop) where there are multiple RP namespaces per org in RPSaaSDev. |
@JeffreyRichter: I was unaware v2 disallowed multiple RP namespaces in a single org, and this is an important detail. Re-reading the spec, I think it mentions this, but it's not emphasized. I will update TSV to enforce this for v2. |
@pallar-ms: We strongly prefer to have checks like TSV work the same in all branches to avoid confusion. Can you change your workflow in RPSaaSDev to use different org folders? If not, and this scenario is important enough, we can discuss options. |
@JeffreyRichter: I think the names matter the least, even |
I agree; your names are fine with me. |
@JeffreyRichter: Just to make sure we're on the same page, there can only be a single resource-manager folder per organization. So when you said "let alone resource-manager folder", these are actually the same restrictions. There must be 0 or 1 resource-manager folders per org, and exactly 1 RP namespace per resource-manager folder. |
|
Yes, you are correct except for the scenario that @pallar-ms points out.
|
I stand by my point: can we not use Management suffix here? it's confusing, if it doesn't matter to anyone else, it matters to me. People keep thinking it was just move the tsp v1 folder xxx.Management into this folder. |
|
|
||
| ```yaml | ||
| openapi-type: arm | ||
| openapi-subtype: rpaas |
There was a problem hiding this comment.
Is it possible to have multiple services in a org folder, but some of those services are public available, and some of those services are still in RPaaS in private preview? In which case, does it make sense to keep this readme.md in public repo here?
There was a problem hiding this comment.
@qiaozha: To whom are you asking this question? There are many people in this PR, so please at-mention whomever you want to respond. Personally, I don't know anything about how public vs private preview works.
There was a problem hiding this comment.
Maybe @pallar-ms your input is needed here? I want to clarify a case like this
specification/widget/resource-manager/foo
specification/widget/resource-manager/bar
where foo service has been public ga or previewed, but bar is still in RPaaS, does it make sense to have
specification/widget/resource-manager/readme.md
to specify the openapi-subtype as rpaas for the whole organization?
There was a problem hiding this comment.
@mikeharder another concern is, this pr is to add example for public repo, the
specification/widget/resource-manager/readme.md file is only required in the https://github.com/Azure/azure-rest-api-specs-pr private repo, I wonder if we could have a pr in private repo to add what's needed in RPaaS?
Otherwise, it's confusing to the public customers.
@qiaozha: Can you please clarify exactly what you disagree with? Currently, the sample has two services, one data-plane named Is your disagreement only that you believe If so, can you please expand on why you think your name is better? I have no opinion, this is between you and @JeffreyRichter. I don't think it matters for any tooling. It's possible having a data-plane and resource-manager "service" with exactly the same name could cause a problem, but I doubt it. |
@mikeharder , sure, that can be one way to address this, but then this implies we have created a distinction in how to perceive 'organization' in this context between RPSaaSDev and RPSaaSMaster. It might not be a big deal but just highlighting the tradeoff. |
@pallar-ms: The problem goes both ways. If we allow multiple RP namespaces in RPSaaSDev but not RPSaaSMaster, this is itself another distinctrion between the branches. This question might best be answered, by whomever is responsible for the requirement to have multiple RP namespaces in RPSaaSDev. What is the source of this requirement? The branch is named Simplest (but slightly dangerous) solution is to make this error suppressable, and suppress it (only for the specs needing it) in RPSaaSDev. Risk is that a spec author might carry this suppression to RPSaaSMaster and main, and reviewers might miss it. |
|
With respect to 1 or more RP Namespaces, I'd say just let the repo work the way it always has worked. |
@JeffreyRichter: OK, but earlier you suggested the opposite. Did this example change your mind?
|
|
From my perspective, there should only be 1 RP per organization. I never knew that some teams create >1 RP namespace for testing purposes in some special branch. Since I don't know much about this scenario, I don't want to block it. Hence, I'm saying don't do anything different than what has been done before. Also, I've never seen 2+ RP namespaces in an organization in the public repo and so this is not a problem that needs fixing from my perspective. I'd like to make less work for you so we can complete this effort and move on. |
@JeffreyRichter: My understanding, the whole point of this effort is to:
I can try to minimize 2, but anything we don't enforce now, might just be a bigger problem later. For this specific point, sure, I'll allow multiple RP namespaces in all branches. |
@JeffreyRichter: TypeSpecValidation and Avocado should be mostly unblocked. It appears Avocado is failing on the partial duplicate readme required by RPaaS. I'll see if I can add a few lines to the extra readme.md to make Avocado happy. @raych1 is looking at all the "SDK Validation - Lang" failures. I assume we don't want to enable any of this, until we have verified SDK generation works correctly from v2. My understanding, SDK generation made assumptions about the relationship between TSP and swagger folders, that now needs to be updated. I'd also like to test the changes @pallar-ms made to a real service. I'm working on this, but since their spec is in the private repo, it's a little more work to test. I agree some of the details about the widget example itself (naming, number of services, etc) can wait. But we're still blocked on SDK validation/generation. |
|
@mikeharder both JS and Go succeeded in the SDK generation, one more, .NET SDK generation succeeded too. These are the issues created across five languages: |
Yes, this is what I want and Jeffrey has expressed somewhere in this thread that the Management suffix was added only to differentiate the name of data plane service name, he doesn't have a strong opinion on this. I want to propose the below name if we really want to differentiate the name and avoid the confusions for everyone.
|
|
/azp run SDK Validation - .NET |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
We cannot have a data plane service called widget and a control plane service also called widget. This would be too confusing for customers in docs and in SDK package names. While we could do this in the specs repo, we should not because the service name folders should be similar to the actual service names. I didn't want to ask someone to look inside the "widget folder" in the specs repo and there be confusion because there are multiple widget folders.
________________________________
From: Qiaoqiao Zhang ***@***.***>
Sent: Wednesday, June 4, 2025 6:45:50 PM
To: Azure/azure-rest-api-specs ***@***.***>
Cc: Jeffrey Richter ***@***.***>; Mention ***@***.***>
Subject: Re: [Azure/azure-rest-api-specs] [EngSys] Add sample "widget" using folder structure v2 (PR #34823)
@qiaozha commented on this pull request.
________________________________
In specification/widget/resource-manager/readme.md<#34823 (comment)>:
@@ -0,0 +1,10 @@
+# widget
+
+## Configuration
+
+Required if any services under this folder are RPaaS.
+
+```yaml
+openapi-type: arm
+openapi-subtype: rpaas
@mikeharder<https://github.com/mikeharder> another concern is, this pr is to add example for public repo, the
specification/widget/resource-manager/readme.md file is only required in the https://github.com/Azure/azure-rest-api-specs-pr private repo, I wonder if we could have a pr in private repo to add what's needed in RPaaS?
Otherwise, it's confusing to the public customers.
—
Reply to this email directly, view it on GitHub<#34823 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AARLJP5LOOA4OWHZ3AWVPCD3B6OM5AVCNFSM6AAAAAB5SCS6A6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDQOJYGY2DENBRGM>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
|
Also, I think the control plane service should (usually) have management in their service name. If I have a "keyvault service" would you naturally think of this as being control plane or data plane? If this is the control plane, then what do you call the data plane service? But, keyvault management service is the control plane then keyvault service is the days plane. If all control plane services end with "management service", then customers know what this means and ARM stands for azure resource manager do there is a strong relationship between all these things using manager/management. The term "Data plane" doesn't have as strong a connotation. In fact, I'm not sure customers know the term "Data plane" ; this may be an internal only term. |
@JeffreyRichter If we take a look at the rest api reference, there's no management wording in there for most of the mgmt services https://learn.microsoft.com/en-us/rest/api/aks/ and I remember we have an assumption to ask service team to use the service name for rest api reference in the future? if that's the case, then Management suffix is also not necessary here. I feel like the people who are referring rest api reference doesn't really care if it's mgmt plane or data plane, they care about the endpoint but the actual value doesn't really matter to them. But the people who are reading the rest api specs like we do trouble shooting or reviewing the spec pr do care, but the resource-manager/data-plane folder has already implied that this is a mgmt plane service, right? Another point is, in my proposal, I have give them a different name where we call the service as a WidgetAnalytics in data plane, and Widget in mgmt plane, does it make sense? if no, why? |
Overview
Swagger BreakingChangedue to the renamesBrowse Spec
https://github.com/mikeharder/azure-rest-api-specs/tree/contoso-all-v2/specification/widget
Open Issues
Verification Builds
Required tooling changes
Related
Outdated