Add Github Actions step for generating docs #2954
Conversation
WalkthroughThis update modifies the GitHub Actions workflow for Node.js CI by adding two new jobs: Changes
Possibly related PRs
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
| - run: npm run build | ||
|
|
||
| - name: Generate Documentation | ||
| run: npm run docgen |
There was a problem hiding this comment.
Thanks for this PR! I think this will just make sure that docgen runs. js.xrpl.org still picks up the files from /docs folder of the main branch. Can you try this suggestion to publish a site from generated artifacts of CI pipeline?
There was a problem hiding this comment.
thanks. I have updated the code here: 2ed20e1
I'm not able to test the "deploy" step via the workflow. Github does not allow me to deploy from non-main branch. Do you have any ideas on how we can fix that?
The generated artifact looks correct to me, I have handpicked some of the files and verified it's contents.
There was a problem hiding this comment.
I think we can try merging ckeshava:genDocs branch in your fork's main branch and see if the GitHub Pages is deployed or not.
There was a problem hiding this comment.
I have not configured Github Pages website to deploy the documentation ://
There was a problem hiding this comment.
Since the generated documentation artifacts are correct (pending PR reviewers approval), only the "deploy-pages" could fail with an unknown error. However, this does not place incorrect docs in the public domain. The only risk is that the docs are not in-sync with the main branch.
In case of such an unknown error, we will need to propose a fix PR and merge that into the main branch.
What do you think of this method?
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/nodejs.yml (1)
198-246: New Documentation Generation Job – Validate Build and Docgen StepsThe addition of the
generate-documentationjob is a solid approach to automate documentation generation on every commit. A few points to verify:
- Command Accuracy: Ensure that
npm run docgencorrectly builds the documentation into the expecteddocs/directory. Verify that the build step (npm run build) preceding it is necessary for the doc generation or if it could be omitted to speed up the workflow.- Caching & Environment: The reuse of Node.js setup, npm version 10, and caching configurations is consistent with other jobs. This promotes consistency across your CI jobs.
If the build step isn’t required for generating documentation, consider removing it to reduce execution time.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/nodejs.yml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: integration (22.x)
- GitHub Check: browser (18.x)
- GitHub Check: integration (20.x)
- GitHub Check: integration (18.x)
🔇 Additional comments (1)
.github/workflows/nodejs.yml (1)
247-257:❓ Verification inconclusive
Deploy Job – Verify Environment URL Output Reference
The deploy job specifies an environment URL as
url: ${{ steps.docs-artifact.outputs.page_url }}However, please ensure that:
- The
actions/deploy-pages@v4step (with iddocs-artifact) properly produces an output namedpage_url.- If the intended URL comes from the output of the
generate-documentationjob, you may need to propagate that value as a job output (e.g., vianeeds.generate-documentation.outputs) rather than referencing a step in the current deploy job.This misreference might result in an undefined URL for the GitHub Pages environment. Please verify that the expected output is available when the deploy job starts executing.
Action Required: Verify the Environment URL Source
Please double-check that the deploy job’s reference for the environment URL—
url: ${{ steps.docs-artifact.outputs.page_url }}—correctly points to a valid output. In particular:
- Confirm that the
actions/deploy-pages@v4step (with iddocs-artifact) indeed produces an output namedpage_url. According to its documentation, it should provide this output, but it's worth verifying in your specific context.- If the intended URL should come from the
generate-documentationjob instead, consider propagating that value as a job output (e.g., usingneeds.generate-documentation.outputs.page_url) and update the reference accordingly.Please review the current configuration in the workflow to ensure the URL is defined as expected when the deploy job starts executing.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/nodejs.yml (1)
247-259: Deployment to GitHub Pages from artifact is correct and addresses previous feedback.The
deployjob correctly depends on the documentation artifact and deploys it to GitHub Pages only on themainbranch. This resolves the concern from the previous review about deploying from CI artifacts rather than the/docsfolder directly. The use ofactions/deploy-pages@v4is appropriate.Nitpick: For clarity, consider renaming the step to "Deploy Documentation to GitHub Pages" (add "Documentation") to make the workflow intent even clearer.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/nodejs.yml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: snippets (22.x)
- GitHub Check: snippets (18.x)
- GitHub Check: integration (18.x)
- GitHub Check: snippets (20.x)
- GitHub Check: browser (18.x)
🔇 Additional comments (1)
.github/workflows/nodejs.yml (1)
198-246: Automated documentation generation and artifact upload are well-implemented.The
generate-documentationjob follows best practices for CI: it builds, generates docs, and uploads them as an artifact for deployment. This approach is robust and aligns with GitHub's recommendations for Pages deployments from CI artifacts.
| - name: Generate Documentation | ||
| run: npm run docgen | ||
|
|
||
| - name: Upload documentation files as artifact |
There was a problem hiding this comment.
It doesn't look like this PR actually commits the docs changes to the repo
There was a problem hiding this comment.
Yes, that is true.
Additionally, the settings of this repository will be updated. Documentation will be deployed from Github Actions, instead of reading from the docs/ folder (this is the existing behavior)
This PR proposes a different way of deploying the documentation. docs/ folder is obviated by the Github Actions workflow.
|
@Patel-Raj Thanks for investigating the deployment of Github Pages! It was very helpful. I have added your suggestions in 9c885f5 |
| runs-on: ubuntu-latest | ||
| needs: generate-documentation | ||
| # Deploy docs only pushes into the main branch | ||
| if: success() && github.ref == 'refs/heads/main' |
There was a problem hiding this comment.
This should be run on a new published (non-beta) version, not on every commit
There was a problem hiding this comment.
-
On the documentation website, I don't observe a menu of various versions. Usually, softwares allow users to select a version before viewing the relevant docs. Does this need to be configure for Typedoc/Github Pages?
-
I agree with your point:
This should be run on a new published (non-beta) version, not on every commit
However, is there any harm in publishing the docs on every commit? (Provided we specify the necessary disclaimers to the users) ?
Especially, if we don't have the infrastructure to render versioned-documentation, I believe its good to keep the docs fresh.
There was a problem hiding this comment.
This should be run on a new published (non-beta) version, not on every commit
If you feel strongly about it, I can accomplish this with modifications to the if-condition.
There was a problem hiding this comment.
However, is there any harm in publishing the docs on every commit? (Provided we specify the necessary disclaimers to the users) ?
It'll be a lot more confusing if users think there's a feature out because the documentation says so, but it's not released.
We don't necessarily need to have the infra for versioned documentation (though we could try to set something up with readthedocs), but the trigger of this action could just be changed to be on a non-beta tag instead, for example.
There was a problem hiding this comment.
ok. updated in 70d8ced
|
@khancode Please review this PR at a convenient time. |
|
@khancode @pdp2121 can one of you review this PR? @Patel-Raj can you please review this PR again? |
.github/workflows/nodejs.yml
Outdated
|
|
||
| strategy: | ||
| matrix: | ||
| node-version: [18.x] |
There was a problem hiding this comment.
We should use 22 here since node 18 maintenance cycle ended last month
There was a problem hiding this comment.
ok fecab1e
We need to upgrade the dev-dependencies into Node v20 (or) v22. However, dependendabot should alert us about that package.
| if: steps.cache-nodemodules.outputs.cache-hit != 'true' | ||
| run: npm ci | ||
|
|
||
| - run: npm run build |
There was a problem hiding this comment.
Probably should have a name for build
There was a problem hiding this comment.
This step is a part of a job titled generate-documentation. Since I'm not referencing this step anywhere else in the workflow file, I did not add a title. Do you feel strongly about it?
|
@Patel-Raj can you please re-review with your alternate github handle? |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/nodejs.yml (1)
249-272:⚠️ Potential issueCritical: Deployment condition prevents any publishing.
Thedeploy-docs-pagesjob is gated byif: success() && github.ref == 'refs/heads/main', so on tag pushes (where you want to release docs) it never runs—and on branch pushes the semantic tag check never passes. As a result, docs will never be deployed.Proposed fix:
- if: success() && github.ref == 'refs/heads/main' + # Only deploy on semantic version tags + if: success() && startsWith(github.ref, 'refs/tags/v')
🧹 Nitpick comments (1)
.github/workflows/nodejs.yml (1)
200-248: New documentation generation job.
Great addition of thegenerate-documentationjob. It installs Node.js/npm, builds the project, runsnpm run docgen, and uploads thedocs/folder as an artifact.
- Please confirm that
npm run docgenindeed outputs todocs/.- Verify that
actions/upload-pages-artifact@v3is the correct action for storing documentation artifacts.
Optionally, consider adding a job-levelname:for improved readability in the UI.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/nodejs.yml(6 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: mvadari
PR: XRPLF/xrpl.js#2788
File: .github/workflows/nodejs.yml:25-25
Timestamp: 2024-09-26T21:14:56.813Z
Learning: In `.github/workflows/nodejs.yml`, the `build-and-lint` job's actions `actions/setup-node` and `actions/cache` are updated to `@v4`.
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: snippets (20.x)
- GitHub Check: snippets (22.x)
- GitHub Check: integration (22.x)
- GitHub Check: browser (22.x)
- GitHub Check: integration (20.x)
🔇 Additional comments (6)
.github/workflows/nodejs.yml (6)
11-13: Verify trigger configuration covers intended events.
The workflow now runs on pushes tomainand all tag references ('**'). Ensure you intend to generate artifacts on every tag (including non-semantic tags) and that pull requests don’t require documentation previews. If you only want to deploy on semantic version tags, consider narrowing the tag filter (e.g.,v*.*.*).
24-24: Update build-and-lint matrix to Node.js 22.x.
Node.js v18 has been removed from thebuild-and-lintjob matrix, pinning this job to 22.x only, which aligns with current LTS support.
64-64: Update unit test matrix to Node.js 20.x and 22.x.
Theunitjob now runs on Node.js 20.x and 22.x, removing deprecated v18 and ensuring broad LTS coverage.
104-104: Align integration tests with active Node.js versions.
Integration tests now run on Node.js 20.x and 22.x, matching the unit test matrix to avoid environment drift.
155-155: Pin browser tests to Node.js 22.x.
Thebrowserjob has been reduced to Node.js 22.x only, as Node 18 maintenance has ended. This satisfies past feedback to drop older versions.
279-279: Update snippets job matrix to Node.js 20.x and 22.x.
Thesnippetsjob now aligns with others by removing Node.js 18, ensuring consistency across CI.
High Level Overview of Change
This PR aims to generate fresh documentation with every commit. Specifically,
js.xrpl.orgwill host the documentation of the latest version of themainbranch. Other functional aspects of the client library are not affected.Additionally, the settings of this repository will be updated. Documentation will be deployed from Github Actions, instead of reading from the
docs/folder (this is the existing behavior)An artifact pertaining to the new documentation is generated in every CI run. The link can be found in the
Upload documentation files as artifactstep of the Github Actions workflow.Context of Change
Type of Change
Did you update HISTORY.md?
Test Plan
The generated artifact and deployed documentation matches the expectation.