[typespec-validation] Error should be reported while missing 'Management' suffix in the folder name of management plane typespec

[raych1]

Context
Per the typespec folder structure requirement, it says "Management at the end of the service RP-name indicates a management (resource manager) library".
The folder should be like the following example:

Additionally, SDK automation tool depends on the 'Management' suffix in the folder name to differentiate the management plane typespec and data plane typespec to call the proper code generator (MPG or DPG) to generate the SDK.

I found typespec-validation tool has the following code to check folder name for management plane typespec case. Request to extend it to gate on the suffix.

azure-rest-api-specs/eng/tools/typespec-validation/src/rules/folder-structure.ts

Line 56 in d129670

if (packageFolder.includes("Management") && packageFolder.includes("Shared")) {

CC: @mikeharder @weshaggard @weidongxu-microsoft

[mikeharder]

@raych1: Can you please be more specific, and give some examples of specs that should pass and fail the new rule?

Are you just asking for the rule to ensure that, if a folder name contains "Management", that the folder ends with either "Management" or "Management.Shared"?

Or is it more complex, say based on the contents of the folder? Something like "If a *.tsp contains <foo>, then the folder name must end with Management"?

[weidongxu-microsoft]

Just an option:

• If service configure typespec-autorest to output to resource-manager folder (e.g. oracle), this tspconfig.yaml should be in a folder with suffix .Management

[mikeharder]

Just an option:

• If service configure typespec-autorest to output to resource-manager folder (e.g. oracle), this tspconfig.yaml should be in a folder with suffix .Management

This check could go on L84:

azure-rest-api-specs/eng/tools/typespec-validation/src/rules/folder-structure.ts

Lines 77 to 89 in d129670

	if (
	!packageFolder.includes("Shared") &&
	!(await host.checkFileExists(path.join(folder, "tspconfig.yaml")))
	) {
	errorOutput += `Invalid folder structure: Spec folder must contain tspconfig.yaml.`;
	success = false;
	}
	return {
	success: success,
	stdOutput: stdOutput,
	errorOutput: errorOutput,
	};

[raych1]

@mikeharder ,

I agree with @weidongxu-microsoft 's suggestion. I'm not sure if azure-resource-provider-folder is required field to set in tspconfig.yaml for typespec-autorest emitter. We might need to cover the cases if it's not set.

In case of the field not being set, we may verify if the spec PR contains any change to the resource-manager folder.

[mikeharder]

In case of the field not being set, we may verify if the spec PR contains any change to the resource-manager folder.

Should already be covered here:

azure-rest-api-specs/eng/tools/typespec-validation/src/rules/linter-ruleset.ts

Lines 49 to 67 in d129670

	if (rpFolder?.trim()?.endsWith("resource-manager")) {
	requiredRuleset = "@azure-tools/typespec-azure-rulesets/resource-manager";
	} else if (rpFolder?.trim()?.endsWith("data-plane")) {
	requiredRuleset = "@azure-tools/typespec-azure-rulesets/data-plane";
	} else if (clientTspExists && !mainTspExists) {
	// Assume folders with no autorest setting, containing only "client.tsp" but no "main.tsp",
	// are data-plane (e.g. HealthInsights.TrialMatcher)
	requiredRuleset = "@azure-tools/typespec-azure-rulesets/data-plane";
	} else {
	// Cannot determine if spec is data-plane or resource-manager, so cannot know
	// which linter ruleset is required.
	success = false;
	errorOutput +=
	"tspconfig.yaml must define the following property:\n" +
	"\n" +
	"options:\n" +
	' "@azure-tools/typespec-autorest":\n' +
	' azure-resource-provider-folder: "data-plane" | "resource-manager"\n';
	}

[raych1]

When the rpFolder ends with resource-manager, the corresponding typespec folder should have a suffix of 'Management'.

[mikeharder]

Proposed fix here: #29663

However, we won't be able to merge this, until we fix or suppress any existing violations (like oracle). Check "TypeSpecValidation-All" will let us know how many existing specs in main are failing.

[mikeharder]

6 specs are failing: https://dev.azure.com/azure-sdk/public/_build/results?buildId=3926861&view=results

@raych1, @weshaggard, @konrad-jamrozik: Do you think we should rename all these spec folders to end in .Management?

[konrad-jamrozik]

@mikeharder how about this:

1. add suppression file(s) for these 6 specs (I think we do support this already, yes?)
2. submit a PR to rename these dirs
3. get approval from the owners of the renamed specs

[raych1]

@mikeharder ,
I suggest renaming all the six services identified with incorrect suffix. This renaming will not need config change or impact SDK generation. //CC: @weidongxu-microsoft

In addition, for the RPaaS service RPs, it depends on the folder path to fetch the spec to do the API call live validation. Therefore, the renaming will require the RP registration update in RPaaS metaRP side. We might need individual PRs for the RpaaS service RP so that service team can determine the proper merge time.

[mikeharder]

@mikeharder how about this:

1. add suppression file(s) for these 6 specs (I think we do support this already, yes?)
2. submit a PR to rename these dirs
3. get approval from the owners of the renamed specs

I generally agree, however:

1. TSV currently only supports supressing all of TSV. You can't suppress individual rules (or in this case, part of a rule). So [TypeSpecValidation] Enforce ".Management" suffix #29663 will be blocked by [TypeSpec Validation] Add per-rule suppression mechanism #24955, but I will prioritize both of these. This is a good forcing function.

2. It sounds like we should use one PR per spec. Since this is a simple rename and only 6 specs, and it was our miss for not checking this earlier, I'm fine creating these PRs myself. I can actually create these PRs in parallel with updating TSV.