-
Notifications
You must be signed in to change notification settings - Fork 5.4k
[LintDiff] Skip readme.md files with no "input-file:" string #35174
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Next Steps to Merge✅ All automated merging requirements have been met! To get your PR merged, see aka.ms/azsdk/specreview/merge. |
PR validation pipeline restarted successfully. If there is ApiView generated, it will be updated in this comment. |
@@ -135,9 +135,18 @@ export async function buildState( | |||
// For readme files that have changed but there are no affected swaggers, | |||
// add them to the map with no tags | |||
for (const changedReadme of existingChangedFiles.filter(readme)) { | |||
const readmePath = resolve(rootPath, changedReadme); | |||
|
|||
// Skip readme.md files that don't have "input-file:" as autorest cannot |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this true? I thought https://aka.ms/autorest
or the yaml blocks were the markers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check is here in autorest:
I did add https://aka.ms/autorest
to the RPaaS readme, to make Avocado pass. Another option here, might be to remove https://aka.ms/autorest
from the RPaaS readme, which might make LintDiff think it's a no-op, but then we need to patch Avocado.
I think the decision point, should this readme.md we need for RPaaS be an "autorest readme" or not? If yes, we make this patch to LintDiff. If no, then we remove the url, and patch Avocado. But patching Avocado might be trickier, because we do want Avocado to fail if the URL is missing from a service readme.
Since autorest fails on the file (due to no input files), maybe it's better to make it a non-autorest file?
Finally, we hope the RPaaS readme is a temporary solution that can be removed in 6 months.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried removing the autorest marker, but LintDiff still fails, so we might as well keep the marker to avoid needing to patch avocado as well.
eng/tools/lint-diff/test/fixtures/buildState/specification/no-input-file/readme.md
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved, but I recommend updating the test to use the real readme.md content. Feel free to add more text to the content to clarify the purpose.
…input-file/readme.md Co-authored-by: Mike Harder <[email protected]>
…zure#35174)" This reverts commit fc12f4e.
…Azure#35174)" This reverts commit e804b25.
This heuristic excludes readme.md files (like the obligate RPaaS readme.md) from scanning
Example existing failure: https://github.com/Azure/azure-rest-api-specs/actions/runs/15546016760?pr=35175
Example with this fix: https://github.com/Azure/azure-rest-api-specs/actions/runs/15546150253
Related PR to legacy LintDiff: https://dev.azure.com/devdiv/DevDiv/_git/openapi-alps/pullrequest/642335