Azure / azure-sdk-tools

Tools repository leveraged by the Azure SDK team.
MIT License
114 stars 180 forks source link

Improve tooling to detect incorrect or missing configs in tspconfig.yaml file during Rest API spec review. #9355

Open lirenhe opened 6 days ago

lirenhe commented 6 days ago

For mgmt. SDK, we require the configurations for all 5 tier 1 languages (Java, Python, .NET, JS and GO) to be present in the tspconfig.yaml file. Here is the sample file: https://github.com/Azure/azure-rest-api-specs/blob/main/specification/contosowidgetmanager/Contoso.Management/tspconfig.yaml

However, today, most service teams only partially update tspconfig.yaml file when they submit their spec PRs for TypeSpec. And It caused 2 problems:

  1. Additional work needed to update tspconfig.yaml before SDK release.
  2. SDK automation may not be triggered for the incorrect tspconfig file so we are not sure the typespec is correct for SDK generation.

We need to improve the tooling to ensure the correctness of tspconfig file.

I will let @lmazuel to comment about the requirement for spec PRs for data plane.

weidongxu-microsoft commented 2 days ago

Java verification

Assume upstream verified

Java automation verifies (we hope this can be verified upstream)

  1. parameter.service-dir.default matches pattern sdk/\w+ (this should be cross-language)
  2. options.@azure-tools/typespec-java.package-dir matches pattern azure(-\w+)+ (package starts with azure-, and not include unexpected char)
  3. options.@azure-tools/typespec-java.namespace matches pattern com\.azure(\.\w+)+ (namespace starts with com.azure., and not include unexpected char)
  4. namespace matches package-dir

(code https://github.com/Azure/azure-sdk-for-java/blob/main/eng/automation/typespec_utils.py)

PS: we may drop (3) and (4) in near future, as namespace is planned to move to client.tsp @clientNamespace decorator.

mikeharder commented 2 days ago

Are you suggesting we add a check, for all management-plane TypeSpec specs, that blocks PRs if "tspconfig.yaml" does not contain entries for all 5 lanugage emitters?  Would the check validate any settings under the language emitters, or just require their presence?

If this check was added, what do you propose doing about all the existing specs that would fail the check?  Perform a bulk update of them, add suppressions and encourage specs owners to update in their next PR, etc?

This check sounds similar to an existing rule we have in tool TypeSpecValidation which validates the autorest emitter settings. So we'd probably want to add this check as a new rule to TypeSpecValidation, rather than a whole new check.

https://github.com/Azure/azure-rest-api-specs/blob/main/eng/tools/typespec-validation/src/rules/emit-autorest.ts

Currently, we have a way to suppress TypeSpecValidation as a whole for a spec (when run as part of TypeSpecValidationAll). We don't have a way to suppress individual rules in TypeSpecValidation, but it could be added fairly easily, if needed to suppress this new rule for existing specs.

raych1 commented 2 days ago

The similar request has been tracked in https://github.com/Azure/azure-rest-api-specs/issues/31043

maririos commented 23 hours ago

The similar request has been tracked in Azure/azure-rest-api-specs#31043

Should this issue be closed as a dup? or what is the difference between them?