-
Notifications
You must be signed in to change notification settings - Fork 14
Description
This readme incorrect used windows paths (backslash) instead of posix paths (forward-slash).
Expected:
input-file:
- preview/2024-10-01-preview/pipelineGroups.json
Actual:
input-file:
- preview\2024-10-01-preview\pipelineGroups.json
Strangely, most of Avocado worked correctly, but it caused false positives in rule MISSING_APIS_IN_DEFAULT_TAG.
Workaround is to use posix paths in readme.
Fix is either:
- Fail fast if readme contains windows paths
- Ensure paths are normalized before comparisons
- Most Avocado rules seemed to work correctly, so maybe bug is specific to
MISSING_APIS_IN_DEFAULT_TAG.
- Most Avocado rules seemed to work correctly, so maybe bug is specific to
Root Cause
I believe the root cause is in dep @azure/openapi-markdown. This returns the input-file "strings", but doesn't attempt to resolve or normalize them:
We might be able to fix/workaround in Avocado, by either normalize (or validating) paths after calling this API:
Lines 209 to 216 in 0576dad
| export const getSwaggerFileUnderDefaultTag = (m: md.MarkDownEx): string[] => { | |
| const defaultTag = getDefaultTag(m.markDown) | |
| if (!defaultTag) { | |
| return [] | |
| } | |
| const inputFiles = openApiMd.getInputFilesForTag(m.markDown, defaultTag) | |
| return (inputFiles as any) || [] | |
| } |
Lines 392 to 402 in 0576dad
| const inputFiles = getSwaggerFileUnderDefaultTag(m) | |
| let defaultTagPathTable = new Map<string, { apiVersion: string; swaggerFile: string }>() | |
| let stableCheck = false | |
| let previewCheck = false | |
| for (const inputFile of inputFiles) { | |
| stableCheck = stableCheck || inputFile.includes('stable') | |
| previewCheck = previewCheck || inputFile.includes('preview') | |
| const inputFilePath = path.resolve(readmeDir, inputFile) | |
| const pathTable = getPathTableFromSwaggerFile(inputFilePath) | |
| defaultTagPathTable = mergePathTable(defaultTagPathTable, pathTable) | |
| } |
Another question, if a file doesn't exist, would it be better for Avocado to throw immediately, rather than silently using an empty map?
Lines 491 to 494 in 0576dad
| export const getPathTableFromSwaggerFile = (swaggerFile: string): PathTable => { | |
| if (!fs.existsSync(swaggerFile)) { | |
| return new Map<string, { apiVersion: string; swaggerFile: string }>() | |
| } |
Our new code in spec-model attempts to normalize and resolve, so it should always return platform-specific absolute paths. Though even this code might be sub-optimal or have bugs.
Metadata
Metadata
Assignees
Labels
Type
Projects
Status