-
Notifications
You must be signed in to change notification settings - Fork 61
feat(docs): api-extractor changes and gh workflow update #303
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
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #303 +/- ##
==========================================
+ Coverage 85.49% 85.55% +0.06%
==========================================
Files 219 219
Lines 24103 24103
Branches 2585 2594 +9
==========================================
+ Hits 20607 20622 +15
+ Misses 3445 3428 -17
- Partials 51 53 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| const apiExtractorDocsTask = project.addTask('api-extractor-docs', { | ||
| exec: [ | ||
| // Ensure API Extractor is installed | ||
| 'npm install -g @microsoft/api-extractor || true', |
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.
Don't use a global install, just add it as devDep
| // Ensure API Extractor is installed | ||
| 'npm install -g @microsoft/api-extractor || true', | ||
| // Run api-extractor to generate the API model | ||
| // Use || true to ensure the task continues even if api-extractor reports failures |
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.
This seems counter intuitive? Why continue on failure?
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.
api-extractor will still output a file when there are certain failures. Will work on fixing the underlying issue, but I did this to temporarily unblock it.
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.
Below that, we check whether the file was outputted instead of relying on api-extractor to reliably report an error.
| 'if [ -f dist/toolkit-lib.api.json ]; then cp dist/toolkit-lib.api.json dist/api-extractor-docs/cdk/api/toolkit-lib/; else echo "Warning: API JSON file not found"; fi', | ||
| // Add version file | ||
| '(cat dist/version.txt || echo "latest") > dist/api-extractor-docs/cdk/api/toolkit-lib/VERSION', | ||
| // Find and copy all markdown files (excluding node_modules) |
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 reckon we can limit this to all files in the docs directory and README.md
You will probably want all files because we might have images.
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.
Got it, that makes sense to me. I was thinking we might have .md files in subdirectories, but if that isn't the case, then we won't need anything outside of docs and README.md. And will include all file types.
|
|
||
| const safeName = this.project.name.replace('@', '').replace('/', '-'); | ||
|
|
||
| releaseWf.addJob(`${safeName}_release_api_extractor_docs`, { |
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.
This seems to be a replication of the S3 docs workflow. Let's refactor that code to be reusable and support both uploads
| "docModel": { | ||
| "enabled": true, | ||
| "apiJsonFilePath": "./dist/<unscopedPackageName>.api.json", | ||
| "projectFolderUrl": "https://github.com/aws/aws-cdk-cli" |
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.
not this?
| "projectFolderUrl": "https://github.com/aws/aws-cdk-cli" | |
| "projectFolderUrl": "https://github.com/aws/aws-cdk-cli/tree/main/packages/%40aws-cdk/toolkit-lib" |
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.
Will change it to this.
mrgrain
left a comment
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.
left some comments
bacdfda to
bbbedb9
Compare
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.
Copilot reviewed 4 out of 11 changed files in this pull request and generated no comments.
Files not reviewed (7)
- packages/@aws-cdk/toolkit-lib/.gitattributes: Language not supported
- packages/@aws-cdk/toolkit-lib/.gitignore: Language not supported
- packages/@aws-cdk/toolkit-lib/.projen/deps.json: Language not supported
- packages/@aws-cdk/toolkit-lib/.projen/files.json: Language not supported
- packages/@aws-cdk/toolkit-lib/.projen/tasks.json: Language not supported
- packages/@aws-cdk/toolkit-lib/api-extractor.json: Language not supported
- packages/@aws-cdk/toolkit-lib/package.json: Language not supported
Comments suppressed due to low confidence (3)
projenrc/s3-docs-publishing.ts:100
- [nitpick] Consider extracting the computed role-session-name into a constant to reduce duplication across steps and mitigate potential inconsistencies.
role-session-name: `s3-${isApiExtractor ? 'api-' : ''}docs-publishing@aws-cdk-cli`
.github/workflows/release.yml:1219
- [nitpick] The S3_PATH is hardcoded for the API Extractor artifact and does not use the dynamic naming logic from S3DocsPublishing; consider aligning these values for consistency.
S3_PATH="$DOCS_STREAM/aws-cdk-toolkit-lib-api-model-v$(cat dist/version.txt).zip"
.projenrc.ts:1057
- Using '|| true' after running api-extractor may suppress critical error output; verify that this handling is intentional and that errors aren’t being inadvertently masked.
'api-extractor run --diagnostics || true'
|
Total lines changed 1870 is greater than 1000. Please consider breaking this PR down. |
Fixes #367
Uses projen to generate the api-extractor config.
Adds
api-extractorJSON output and .md files to the S3 bucket for docs during release.Does not affect the existing TypeDoc site workflow.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license