Skip to content
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

Update the specification directory structure article #28464

Merged
merged 18 commits into from
Apr 5, 2024

Conversation

konrad-jamrozik
Copy link

@konrad-jamrozik konrad-jamrozik commented Mar 26, 2024

Link for easy review of the resulting document:

A follow-up to:

As I continue to work on overhauling our documentation.

This PR contributes to:

Notable changes:

  • I removed a lot of details about service groups and added a note we do not support new service group going forward, just existing ones.
    • There is a lot of related discussion in the email thread with subject RE: Typespec questions
  • I deduped some information about TypeSpec and instead linked to relevant articles.
  • I removed mention of profile. The repository has profile and profiles folders but these directories appear to be legacy to me, untouched for 3 years.
  • I removed mention of "component entities" from several places. I believe this was related to the fact there was a 1 to 1 mapping between ARM services and Service Tree service components.
  • I removed the mention that a GA can have a component preview. This should not be allowed.

Additional notes:

TODOs:

Copy link

openapi-pipeline-app bot commented Mar 26, 2024

Next Steps to Merge

✅ All automated merging requirements have been met! To get your PR merged, see aka.ms/azsdk/specreview/merge.

Copy link

openapi-pipeline-app bot commented Mar 26, 2024

Swagger Validation Report

️️✔️BreakingChange succeeded [Detail] [Expand]
There are no breaking changes.
️️✔️Breaking Change(Cross-Version) succeeded [Detail] [Expand]
There are no breaking changes.
️️✔️CredScan succeeded [Detail] [Expand]
There is no credential detected.
️️✔️LintDiff succeeded [Detail] [Expand]
Validation passes for LintDiff.
️️✔️Avocado succeeded [Detail] [Expand]
Validation passes for Avocado.
️️✔️SwaggerAPIView succeeded [Detail] [Expand]
️️✔️TypeSpecAPIView succeeded [Detail] [Expand]
️️✔️ModelValidation succeeded [Detail] [Expand]
Validation passes for ModelValidation.
️️✔️SemanticValidation succeeded [Detail] [Expand]
Validation passes for SemanticValidation.
️️✔️PoliCheck succeeded [Detail] [Expand]
Validation passed for PoliCheck.
️️✔️SpellCheck succeeded [Detail] [Expand]
Validation passes for SpellCheck.
️️✔️Lint(RPaaS) succeeded [Detail] [Expand]
Validation passes for Lint(RPaaS).
️️✔️PR Summary succeeded [Detail] [Expand]
Validation passes for Summary.
️️✔️Automated merging requirements met succeeded [Detail] [Expand]
Posted by Swagger Pipeline | How to fix these errors?

Copy link

openapi-pipeline-app bot commented Mar 26, 2024

Swagger Generation Artifacts

️️✔️ApiDocPreview succeeded [Detail] [Expand]
Posted by Swagger Pipeline | How to fix these errors?

Copy link

openapi-pipeline-app bot commented Mar 26, 2024

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

TypeSpec folders are `specification/playfab/Playfab`.
`rpnamespace` names present are: `Microsoft.Automation` (in `automation` rp), `Microsoft.Batch` (in `batch` rp) and `Microsoft.Playfab` (in `playfab` rp).

## Folder Structure for service groups
Copy link
Author

@konrad-jamrozik konrad-jamrozik Mar 26, 2024

Choose a reason for hiding this comment

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

@JeffreyRichter @rkmanda I significantly shortened this section and added a note of this not being allowed going forward. Can you review?

Copy link
Member

Choose a reason for hiding this comment

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

Lets setup a meeting and discuss.

@konrad-jamrozik konrad-jamrozik added the Central-EngSys This issue is owned by the Engineering System team. label Mar 26, 2024
documentation/directory-structure.md Outdated Show resolved Hide resolved
documentation/directory-structure.md Outdated Show resolved Hide resolved
If TypeSpec sources are present, OpenAPI specs in the `resource-manager`
and `data-plane` folders are generated from TypeSpec sources.

## Resource provider namespace (`rpnamespace`) folders
Copy link
Member

Choose a reason for hiding this comment

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

Should this be higher in the list? before TypeSpec sources?

documentation/directory-structure.md Show resolved Hide resolved
documentation/directory-structure.md Outdated Show resolved Hide resolved
Copy link
Member

@rkmanda rkmanda left a comment

Choose a reason for hiding this comment

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

🕐

@konrad-jamrozik
Copy link
Author

konrad-jamrozik commented Mar 27, 2024

The doc edited in this PR doesn't seem to currently take into account the following special case from the email thread Re: TypeSpecValidation and brownfield specs in RPSaaSMaster by @JeffreyRichter, and should be properly documented here:

Some "services" MIGHT be divided into sub-services; for example: azure-rest-api-specs/specification/hdinsight/resource-manager/Microsoft.HDInsight at main · Azure/azure-rest-api-specs (github.com)
Here Microsoft.HDInsight (the parent) has preview & stable folders.
But there is also a HDInsightsOnAks folder which also has preview & stable.

This would surface to customers a 2 separate Azure services, and each could version independently.
We would ship 1 SDK for HDInsights and a completely separate SDK for HDInsightsOnAks.
This example is OK with me/us.

This probably means I should un-delete & update this part:

If the AutoRest configuration file (aka. the readme.md) is placed out of sub-service/sub-component folders, then there will be only one SDK package that holds all sub-services/sub-components. If the file is placed in each sub-service/sub-component folder, then there will be separate SDK packages of each sub-service/sub-component. Ensure to consult Azure SDK ArchBoard for SDK packaging strategy when consolidating AutoRest configuration file for SDK generation.

@lfraleigh
Copy link
Member

cc @josefree

@maririos
Copy link
Member

maririos commented Apr 4, 2024

FYI @ladonnaq

Copy link
Member

@weshaggard weshaggard left a comment

Choose a reason for hiding this comment

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

Left some feedback but looks good to me overall.

@konrad-jamrozik konrad-jamrozik dismissed rkmanda’s stale review April 5, 2024 18:20

To unblock the PR, as rkmanda doesn't have time to review it. I'll incorporate any feedback in a follow-up PR.


For example, [`specification/containerservice`] is a `service group` for both `aks` and `fleet` services.

The doc for `aks` is [Azure Kubernetes Service]. It points to aks REST reference e.g. for [API version `2024-01-01`][aks REST reference 2024-01-01],
Copy link
Member

Choose a reason for hiding this comment

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

doc for aks is [Azure Kubernetes Service].

will be good to have a link to the doc here

Copy link
Author

Choose a reason for hiding this comment

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

It is indeed hyperlinked. I am using markdown reference links. It links to https://learn.microsoft.com/en-us/azure/aks/


Each `README.md` describes a single `service` and is used as an SDK package and documentation for each version of the service.
Inside the `README.md` file there are lists of paths to OpenAPI spec `.json` files making up given service version.

Copy link
Member

Choose a reason for hiding this comment

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

I think we should describe specific requirements on the README.md here. For example, it should contain a "default" tag that refers to "current" version of the service (the ref docs depend on this).

Copy link
Author

Choose a reason for hiding this comment

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

To avoid duplication I wanted to keep this section lightweight. But perhaps we should add it to https://aka.ms/azsdk/autorest.

Note this doc mentions:

Both the /resource-manager and /data-plane folders must contain an AutoRest configuration file, README.md. Learn more about this file at aka.ms/azsdk/autorest.


The `specification` folder is the root folder for all service specifications.

Each child of the `specification` folder corresponds to a `service` specification for given Azure team. Here we denote such folder as `<azureTeam>`.
Copy link
Member

Choose a reason for hiding this comment

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

I think we should change all instances of "azureTeam" to "azureService". We should not "ship our organization structure" in our public API surface, but using "azureTeam" suggests that we do that.

Copy link
Author

@konrad-jamrozik konrad-jamrozik Apr 15, 2024

Choose a reason for hiding this comment

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

@mikekistler can you consult this with @JeffreyRichter ? My concern is that if we call it azureService then we end up with conflicting terminology where an azureService has inside it a serviceGroup in some cases, which IMO is very counter-intuitive.

wiboris pushed a commit to wiboris/azure-rest-api-specs that referenced this pull request May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Central-EngSys This issue is owned by the Engineering System team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants