Skip to content

[contoso] Move swagger and readme for folder structure v2 #34677

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

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

mikeharder
Copy link
Member

@mikeharder mikeharder commented May 14, 2025

  • Move swagger and readmes to align with folder structure v2
  • Leaves TypeSpec sources unchanged

Changelog

  1. data-plane/Azure.Contoso.WidgetManager renamed to data-plane/Widget
  2. data-plane/readme.md moved one level down to data-plane/Widget/readme.md
  3. resource-manager/Microsoft.Contoso renamed and moved one level down to resource-manager/Contoso.Widget/WidgetManagement
  4. Relative paths in swagger files under resource-manager/Contoso.Widget/WidgetManagement updated per new folder depth
  5. resource-manager/readme.md moved two levels down to resource-manager/Contoso.Widget/WidgetManagement/readme.md
  6. resource-manager/readme.md replaced with new content related to "ARM schema validation"

Questions

  1. Should the new resource-manager/readme.md be ignored by all existing swagger tooling?

Copy link

openapi-pipeline-app bot commented May 14, 2025

Next Steps to Merge

Next steps that must be taken to merge this PR:
  • ❌ This PR targets either the main branch of the public specs repo or the RPSaaSMaster branch of the private specs repo. These branches are not intended for iterative development. Therefore, you must acknowledge you understand that after this PR is merged, the APIs are considered shipped to Azure customers. Any further attempts at in-place modifications to the APIs will be subject to Azure's versioning and breaking change policies. Additionally, for control plane APIs, you must acknowledge that you are following all the best practices documented by ARM at aka.ms/armapibestpractices. If you do intend to release the APIs to your customers by merging this PR, add the PublishToCustomers label to your PR in acknowledgement of the above. Otherwise, retarget this PR onto a feature branch, i.e. with prefix release- (see aka.ms/azsdk/api-versions#release--branches).
  • ❌ This PR has at least one breaking change (label: BreakingChangeReviewRequired).
    To unblock this PR, follow the process at aka.ms/brch.
  • ❌ The required check named TypeSpec Validation has failed. Refer to the check in the PR's 'Checks' tab for details on how to fix it and consult the aka.ms/ci-fix guide

Copy link

openapi-pipeline-app bot commented May 14, 2025

PR validation pipeline restarted successfully. If there is ApiView generated, it will be updated in this comment.

Copy link

github-actions bot commented May 14, 2025

API Change Check

APIView identified API level changes in this PR and created the following API reviews

Language API Review for Package
Swagger Contoso.Widget-WidgetManagement
Swagger Widget-Widget

@mikeharder mikeharder requested a review from weshaggard May 14, 2025 06:32
@mikeharder mikeharder self-assigned this May 14, 2025
@mikeharder mikeharder moved this from 🤔 Triage to 🐝 Dev in Azure SDK EngSys 🥧📅 May 14, 2025
@AzureRestAPISpecReview AzureRestAPISpecReview added BreakingChangeReviewRequired <valid label in PR review process>add this label when breaking change review is required data-plane new-rp-namespace resource-manager RPaaS SuppressionReviewRequired TypeSpec Authored with TypeSpec labels May 14, 2025
@@ -0,0 +1,48 @@
# containerstorage
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this readme related to the one in the WidgetManagment folder?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The readme.md file in the Contoso.Widget folder is the union of the content in each service name folder's readme.md file. This union file is required by RPSaaS. I just learned yesterday that they will get rid of the need for this file "someday".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it be an import of the other files instead of a copy of the content?

Also, I suspect there may be other tools that require the readme at this level as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the RPaaS schema validator will work if the top-level readme file imports (references) the lower-level readme files. I don't think other tools need this top-level readme because I had one team remove it entirely (because I didn't know about the RPaaS schema validation checker) and the schema validation checker was the only thing that broke.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also know that docs & SDKs will refer to the lower-level readmes and not the top-level readme.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JeffreyRichter: I'm still confused. If the schema validation already works with the folder structure v1, why is the extra (merged) readme.md necessary for folder structure v2?

For resource-manager swagger and readmes, I thought your claim was the repo already supports both v1 and v2 (without the extra readme). Some resource-manager specs using a single level of nesting (v1), but some already use two levels of nesting (I believe this is identical to v2, minus the extra readme.md).

So what exactly is introducing the requirement for the extra (merged) readme.md?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or is splitting (and re-merging) the readmes the entire point of folder structure v2? For example, this spec already has two levels of folders under resource-manager, but only a single readme.md directly under resource-manager.

https://github.com/Azure/azure-rest-api-specs/tree/main/specification/compute/resource-manager

So, while this spec already has their swaggers using the v2 folder structure, their readme.md is still using the v1 structure? Is this why this spec works with "schema validation" today, but requires an extra (merged) readme after the readmes are split and moved down into sub-folders?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The high priority goal is to make each service self-contained.
So, each service has its own folder and ALL things related to this service are in that folder: tsp files, preview/stable swaggers, readme.md files, etc. And from these files, docs & SDKs are produced. If a service is later retired, then we can just delete the service folder and everything related to that service is deleted in one fell swoop.

For control-plane services, they are under an extra folder: the RP namespace folder (eg: Microsoft.Compute). data-plane doesn't have this extra folder. And, control-plane services all need this extra readme.md file because of RPaaS schema validation which validates all operations within the RP namespace regardless of how the operations are split across multiple services. The RPaaS team told me that they hope to get rid of the requirement for this extra readme.md file in the future so this is required as a stop gap until then.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, so it's what I said in my last paragraph. Some specs already in main split their swaggers into self-contained folders, but they still use a single readme.md at the top-level of resource-manager.

For these specs (like compute), the only change in v2 will be splitting the existing readme.md into multiple files, one per "service", and one extra readme per "RP namespace" (hopefully temporary).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will all the content need to be duplicated in those split readmes? Or only part of the info? We need to figure out what is needed in each type of readme to ensure the duplication and confusion is minimal.

@mikeharder mikeharder moved this to 🔬 Dev in PR in Azure SDK EngSys 🥧📅 May 15, 2025
@mikeharder mikeharder moved this from 🔬 Dev in PR to 🐝 Dev in Azure SDK EngSys 🥧📅 May 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BreakingChangeReviewRequired <valid label in PR review process>add this label when breaking change review is required data-plane new-rp-namespace resource-manager RPaaS SuppressionReviewRequired TypeSpec Authored with TypeSpec
Projects
Development

Successfully merging this pull request may close these issues.

4 participants