Fixing container service warning and arm-types reference #24044

allenjzhang · 2023-05-18T03:14:38Z

Added arm-types-dir so the common type reference built in rest-api-repo is correct
Kept the CreateOrUpdate operation id via suppress. Without this override, it generated Fleet_Create.
Updated tspconfig.yaml to align with repo convention

Passing build in pipeline specs - typespec - ci: https://dev.azure.com/azure-sdk/public/_build/results?buildId=2785861&view=logs&j=9cd39ef8-47c1-5b58-08db-8c3708712f1b&t=462145f3-d740-50de-51e9-126e7c0a29e2

TypeSpecValidation is showing an error (even though it reports green status), but this is a bug where TypeSpecValidation can't handle multiple versions of the generated swagger in the repo. It should only be comparing the generated swagger against version 2023-03-15, but it tries to compare against 2022-09-02 instead, which fails.

openapi-workflow-bot · 2023-05-18T03:14:41Z

Hi, @allenjzhang Thanks for your PR. I am workflow bot for review process. Here are some small tips.

Please ensure to do self-check against checklists in first PR comment.

PR assignee is the person auto-assigned and responsible for your current PR reviewing and merging.

For specs comparison cross API versions, Use API Specs Comparison Report Generator

If there is CI failure(s), to fix CI error(s) is mandatory for PR merging; or you need to provide justification in PR comment for explanation. How to fix?

Any feedback about review process or workflow bot, pls contact swagger and tools team. [email protected]

openapi-pipeline-app · 2023-05-18T03:14:43Z

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.

️️✔️ApiReadinessCheck succeeded [Detail] [Expand]

️️✔️~[Staging] ServiceAPIReadinessTest succeeded [Detail] [Expand]

Validation passes for ServiceAPIReadinessTest.

️️✔️SwaggerAPIView succeeded [Detail] [Expand]

️️✔️CadlAPIView 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.

️️✔️PrettierCheck succeeded [Detail] [Expand]

Validation passes for PrettierCheck.

️️✔️SpellCheck succeeded [Detail] [Expand]

Validation passes for SpellCheck.

️️✔️Lint(RPaaS) succeeded [Detail] [Expand]

Validation passes for Lint(RPaaS).

️️✔️CadlValidation succeeded [Detail] [Expand]

Validation passes for CadlValidation.

️️✔️TypeSpec Validation succeeded [Detail]

Rule	Message
`❌` InConsistentSwagger	"details":"The generated swagger file 2022-09-02-preview/fleets.json from typespec specification/containerservice/Fleet.Management is not the same as the '/mnt/vss/_work/1/azure-rest-api-specs/specification/containerservice/resource-manager/Microsoft.ContainerService/aks/preview/2022-09-02-preview/fleets.json' in PR, please make sure the swagger is consistent with the generated swagger. You can find the difference in the pipeline log."
`❌` InConsistentSwagger	"details":"The generated swagger file 2023-03-15-preview/fleets.json from typespec specification/containerservice/Fleet.Management is not the same as the '/mnt/vss/_work/1/azure-rest-api-specs/specification/containerservice/resource-manager/Microsoft.ContainerService/fleet/preview/2023-03-15-preview/fleets.json' in PR, please make sure the swagger is consistent with the generated swagger. You can find the difference in the pipeline log."

️️✔️PR Summary succeeded [Detail] [Expand]

Validation passes for Summary.

_{Posted by Swagger Pipeline | How to fix these errors?}

openapi-pipeline-app · 2023-05-18T03:14:43Z

Swagger pipeline restarted successfully, please wait for status update in this comment.

openapi-pipeline-app · 2023-05-18T03:14:44Z

Generated ApiView

Language	Package Name	ApiView Link
TypeSpec	Fleet.Management	https://apiview.dev/Assemblies/Review/8857ba5792294341a33f254fab0a130d

weshaggard · 2023-05-19T15:00:06Z

specification/containerservice/Fleet.Management/tspconfig.yaml

@@ -2,7 +2,9 @@ emit:
  - '@azure-tools/typespec-autorest'
 options:
  '@azure-tools/typespec-autorest':
-    azure-resource-provider-folder: '{cwd}/../resource-manager'
+    arm-types-dir: ../../../../../../common-types/resource-management


Is this necessary? That path looks like a recipe for trouble.

Suggested change

arm-types-dir: ../../../../../../common-types/resource-management

arm-types-dir: "{project-root}/../../common-types/resource-management"

it should be using {project-root} (not sure I fixed it correctly here)

We should not have a relative path here. We should use a path starting from "{project-root}" https://github.com/Azure/typespec-azure/blob/main/packages/typespec-autorest/src/openapi.ts#L799-L808

I merged @timotheeguerin suggestion, CI pipeline will tell us if it works.

Does this mean that folks with this reference will not be able to run outside of the specs repo? For the most part we have blocked references outside of the service directory but this will be one which goes outside of it.

nvm, repro'd I copied the wrong tspconfig

fix here https://github.com/Azure/typespec-azure/pull/3025

We can remove this config then as it is defualt value. I will remove it from the template definitely.

I will create a PR to upgrade the specs repo to use the new autorest emitter (and verify it doesn't regress any specs currently building)

PR to update tools: #24100

Pipeline specs - typespec - ci is passing, which means no spec in the repo was currently relying on the buggy behavior. So we can either:

Merge this PR, then 24100, then fix the usage of {project-root} in a third PR

Merge 24100, then fix the usage of {project-root} in this PR, then merge this PR

markcowl · 2023-05-19T20:40:09Z

specification/containerservice/Fleet.Management/tspconfig.yaml

@@ -2,7 +2,9 @@ emit:
  - '@azure-tools/typespec-autorest'
 options:
  '@azure-tools/typespec-autorest':
-    azure-resource-provider-folder: '{cwd}/../resource-manager'
+    arm-types-dir: ../../../../../../common-types/resource-management


We should not have a relative path here. We should use a path starting from "{project-root}" https://github.com/Azure/typespec-azure/blob/main/packages/typespec-autorest/src/openapi.ts#L799-L808

Co-authored-by: Timothee Guerin <[email protected]>

…fig.yaml" This reverts commit 6896819.

mikeharder · 2023-05-19T23:15:36Z

@timotheeguerin, @markcowl: Think this is ready to merge, given this is the best we can do until https://github.com/Azure/typespec-azure/issues/3024 is fixed?

mikeharder · 2023-05-20T01:57:10Z

/azp run

azure-pipelines · 2023-05-20T01:57:22Z

Azure Pipelines successfully started running 2 pipeline(s).

timotheeguerin · 2023-05-22T14:53:31Z

@mikeharder, made a hotfix friday for the project-root issue, waiting on the pr bumping the versions and the hotfix should be usable https://github.com/Azure/typespec-azure/pull/3026

Because technically that bug breaks a bad use, there could be some specs that need updating though so if you prefer to merge this for now with relative ref, I have no issue

allenjzhang · 2023-05-22T22:12:06Z

Discussed with Stephane, he will send a PR from his side that incorporate these small changes rather than merging. So closing this.

allenjzhang · 2023-05-23T00:03:21Z

Actually, he is not merging into the main and won't have time for now. So let's merge this PR then. Reactivated.

allenjzhang · 2023-05-23T00:04:58Z

@mikeharder, made a hotfix friday for the project-root issue, waiting on the pr bumping the versions and the hotfix should be usable Azure/typespec-azure#3026

Because technically that bug breaks a bad use, there could be some specs that need updating though so if you prefer to merge this for now with relative ref, I have no issue

I think it is best to have {project-root}/../xxx for now. Once the hotfix version is updated, we can do a full pass on the repo and correct other instances all together.

allenjzhang · 2023-05-23T00:07:31Z

@markcowl, feel free to remove -1 and merge to unblock Mike.

mikeharder · 2023-05-23T00:09:29Z

I think we need to update the version of autorest in the repo to prevent this PR from breaking the CI pipeline. I'll just add the version update to this PR.

mikeharder · 2023-05-23T00:11:27Z

I think we need to update the version of autorest in the repo to prevent this PR from breaking the CI pipeline. I'll just add the version update to this PR.

Sorry I misunderstood. I see the plan is to intentionally use the buggy behavior for {project-root} in this PR, and fix it in a later PR where the version of autorest is updated. Fine with me.

timotheeguerin · 2023-05-23T00:16:11Z

@mikeharder autorest has been updated with the fix. Version 0.30.1

mikeharder · 2023-05-23T00:30:46Z

@mikeharder autorest has been updated with the fix. Version 0.30.1

Yep, I will create a separate PR to update the version of autorest in root package.json to 0.30.1. In this PR, we will need to fix any usage of {project-root} depending on the old behavior (like this current PR).

specification/containerservice/Fleet.Management/tspconfig.yaml

mikeharder · 2023-05-23T01:19:03Z

/azp run

azure-pipelines · 2023-05-23T01:19:15Z

Azure Pipelines successfully started running 2 pipeline(s).

- Setting matches default value

specification/containerservice/Fleet.Management/tspconfig.yaml

mikeharder · 2023-05-23T23:01:54Z

@weshaggard: If you think this is ready to merge, can you approve?

I'd like to get this merged soon if there are no other comments. We can always make additional improvements in future PRs.

Fixing container service warning and arm-types reference

ef0781b

allenjzhang requested review from palma21, weinong, alvinli222, matthchr and circy9 as code owners May 18, 2023 03:14

openapi-assignment-bot bot assigned keryul May 18, 2023

ghost added the Container Service label May 18, 2023

Add Fleet.Management to CI pipeline

ca83b34

mikeharder mentioned this pull request May 18, 2023

[TypeSpec CI] Add Fleet.Management #24043

Closed

Update tspconfig.yaml to align with sample

0a4508a

mikeharder approved these changes May 18, 2023

View reviewed changes

weshaggard reviewed May 19, 2023

View reviewed changes

markcowl requested changes May 19, 2023

View reviewed changes

mikeharder and others added 2 commits May 19, 2023 13:43

Update specification/containerservice/Fleet.Management/tspconfig.yaml

6896819

Co-authored-by: Timothee Guerin <[email protected]>

Revert "Update specification/containerservice/Fleet.Management/tspcon…

82cc31d

…fig.yaml" This reverts commit 6896819.

allenjzhang closed this May 22, 2023

Removing last suppression

010c659

allenjzhang reopened this May 23, 2023

update arm-types-dir to use absolute path. Need fixing it later

05ec4ae

markcowl requested changes May 23, 2023

View reviewed changes

specification/containerservice/Fleet.Management/tspconfig.yaml Outdated Show resolved Hide resolved

Update arm-types-dir to correct path

557e29f

markcowl approved these changes May 23, 2023

View reviewed changes

Remove arm-types-dir

87ccb2b

- Setting matches default value

weshaggard reviewed May 23, 2023

View reviewed changes

specification/containerservice/Fleet.Management/tspconfig.yaml Outdated Show resolved Hide resolved

mikeharder added 2 commits May 23, 2023 22:50

Merge branch 'main' into azhang_FixContainerServiceFleetTypeSpec

9b80ddd

Remove unnecessary "./" in tspconfig.yaml

6dd5f91

weshaggard approved these changes May 24, 2023

View reviewed changes

weshaggard merged commit 89ba26d into Azure:main May 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing container service warning and arm-types reference #24044

Fixing container service warning and arm-types reference #24044

allenjzhang commented May 18, 2023 •

edited by mikeharder

Loading

openapi-workflow-bot bot commented May 18, 2023

openapi-pipeline-app bot commented May 18, 2023 •

edited

Loading

openapi-pipeline-app bot commented May 18, 2023 •

edited

Loading

openapi-pipeline-app bot commented May 18, 2023 •

edited

Loading

weshaggard May 19, 2023

timotheeguerin May 19, 2023

markcowl May 19, 2023 •

edited

Loading

mikeharder May 19, 2023

weshaggard May 19, 2023

timotheeguerin May 19, 2023

timotheeguerin May 20, 2023

allenjzhang May 23, 2023 •

edited

Loading

mikeharder May 23, 2023

mikeharder May 23, 2023

markcowl May 19, 2023 •

edited

Loading

mikeharder commented May 19, 2023

mikeharder commented May 20, 2023

azure-pipelines bot commented May 20, 2023

timotheeguerin commented May 22, 2023

allenjzhang commented May 22, 2023

allenjzhang commented May 23, 2023

allenjzhang commented May 23, 2023

allenjzhang commented May 23, 2023

mikeharder commented May 23, 2023

mikeharder commented May 23, 2023

timotheeguerin commented May 23, 2023

mikeharder commented May 23, 2023

mikeharder commented May 23, 2023

azure-pipelines bot commented May 23, 2023

mikeharder commented May 23, 2023

	arm-types-dir: ../../../../../../common-types/resource-management
	arm-types-dir: "{project-root}/../../common-types/resource-management"

Fixing container service warning and arm-types reference #24044

Fixing container service warning and arm-types reference #24044

Conversation

allenjzhang commented May 18, 2023 • edited by mikeharder Loading

openapi-workflow-bot bot commented May 18, 2023

openapi-pipeline-app bot commented May 18, 2023 • edited Loading

Swagger Validation Report

openapi-pipeline-app bot commented May 18, 2023 • edited Loading

openapi-pipeline-app bot commented May 18, 2023 • edited Loading

Generated ApiView

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markcowl May 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allenjzhang May 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markcowl May 19, 2023 • edited Loading

Choose a reason for hiding this comment

mikeharder commented May 19, 2023

mikeharder commented May 20, 2023

azure-pipelines bot commented May 20, 2023

timotheeguerin commented May 22, 2023

allenjzhang commented May 22, 2023

allenjzhang commented May 23, 2023

allenjzhang commented May 23, 2023

allenjzhang commented May 23, 2023

mikeharder commented May 23, 2023

mikeharder commented May 23, 2023

timotheeguerin commented May 23, 2023

mikeharder commented May 23, 2023

mikeharder commented May 23, 2023

azure-pipelines bot commented May 23, 2023

mikeharder commented May 23, 2023

allenjzhang commented May 18, 2023 •

edited by mikeharder

Loading

openapi-pipeline-app bot commented May 18, 2023 •

edited

Loading

openapi-pipeline-app bot commented May 18, 2023 •

edited

Loading

openapi-pipeline-app bot commented May 18, 2023 •

edited

Loading

markcowl May 19, 2023 •

edited

Loading

allenjzhang May 23, 2023 •

edited

Loading

markcowl May 19, 2023 •

edited

Loading