fix-common-types-relative-path-bug #2832
Conversation
qiaozha
requested review from
bterlson,
markcowl and
timotheeguerin
as code owners
microsoft-github-policy-service
bot
added
the
emitter:autorest
|
All changed packages have been documented.
|
|
You can try these changes here
|
|
@timotheeguerin @markcowl could you help review this pr, it's blocking the folder structure v2 migration.https://github.com/Azure/azure-rest-api-specs/actions/runs/15632462934/job/44039741490?pr=34549 |
| [AutorestTestLibrary.name]: { | ||
| ...options, | ||
| "emitter-output-dir": outputDir, | ||
| "output-file": "openapi.json", |
There was a problem hiding this comment.
if we don't set it explicitly, we will get the default output-file {azure-resource-provider-folder}/{service-name}/{version-status}/{version}/openapi.json, which is not true in this test case.
There was a problem hiding this comment.
how is the change to the code affect this? can you add a test for the new structure. There is a test right below that was testing the default behavior which I assume this then just broke
| - "@azure-tools/typespec-autorest" | ||
| --- | ||
|
|
||
| fix-common-types-relative-path-bug |
There was a problem hiding this comment.
write an actual changelog entry
| const armTypesDir = interpolatePath( | ||
| resolvedOptions["arm-types-dir"] ?? "{project-root}/../../common-types/resource-management", | ||
| resolvedOptions["arm-types-dir"] ?? | ||
| `${specificationIndex > -1 ? "../".repeat(segments.length - specificationIndex - 2) : "{project-root}/../../"}common-types/resource-management`, |
There was a problem hiding this comment.
please don't use relative path here this should resolve alwaays to an absolute path. This will break later otherwise for specific cases. The point of the arm-types-dir is that its supposed to resolve to the absolute path (with just some back compat we never cleaned up) and when we resolve the ref it resolve relative to that.
Can you reexplain what is the v2 structure.
There was a problem hiding this comment.
@timotheeguerin v2 folder structure unifies the TypeSpec and swagger into the same service folder so that service team can easily extends to multiple services. Which is an option we provide for service team to resolve the version uniform issue and many service team tend to choose this option.
The folder structure looks like:
specification/{orgName}/
└── resource-manager|data-plane/
└── {RPNamespace}/ ← if it's data plane, we don't have this RP Namespace folder
└── {serviceName}/ ← Service folder
├── readme.md ← Only readme.md (your current file)
├── readme.language.md ← Other AutoRest related configurations for SDK generation
├── *.tsp files
├── preview/
├── stable/
Here's an widget example for v2 folder structure Azure/azure-rest-api-specs#34823
What I believe is the default output-file has changed from {azure-resource-provider-folder}/{service-name}/{version-status}/{version}/openapi.json into {version-status}/{version}/openapi.json in v2 is causing the problem here. But v1 will exist for quite sometime before we have migrated all the existing folder structure to v2.
And @mikeharder uses the arm-types-dir to make the widget case work, but I feel like this could be fixed by the leveraging the output-file and emitter-output to figure out the relative path to the ARM common types.
Let me know if it makes sense to you.
There was a problem hiding this comment.
@timotheeguerin: I needed to modify tspconfig.yaml in the v2 resource-manager sample, since the relative path to common-types is no longer the same for data-plane and resource-manager.
I think this change is trying to make the default work in folder structure v2? I don't know either way if this is worth it.
There was a problem hiding this comment.
using a relative path is what makes this unreliable, can't we have something that exactly point to where the common-types folder is relative to the main.tsp folder for the v2 structure instead.
There was a problem hiding this comment.
from what I understand in the structure can't it be {project-root}/../../../../common-types
There was a problem hiding this comment.
its the same as the current value, that ref gets resolved automatically. This is the point of wjhy you don't use a relative path to start. Otherwise its broken if its not exactly in teh right place?
There was a problem hiding this comment.
if you setup the file relative to the project root, then it doesn't matter where the output is it will resolve it correctly by resolving getRelativePathFrom(currentOutputFile, refAbsolutePath)
There was a problem hiding this comment.
@timotheeguerin: The path from project root to common-types is different in structures v1 and v2. There is no hardcoded string default that can work for both. It either needs to be a dynamically-calculated default, or we live with a default that only works for v1 (or v2).
There was a problem hiding this comment.
I understand it is different and you need the switch but both cases should have an absolute path. Right now if using v2 it uses some relative path but v1 it uses the absolute one
There was a problem hiding this comment.
just asking that the new path also use something relative to {project-root}
markcowl
left a comment
markcowl
left a comment
There was a problem hiding this comment.
+1 - I think the change to support v2 should be a change to the default config file setting and not a change to path resolution
6 participants
With typespec folder structure v2. the typespec-autorest project output folder is not under specification/{orgName] anymore. We need to calculate the relative path for common types accordingly.