Skip to content
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

feat: github action to integrate/publish endo and agoric-sdk reference docs too #1038

Merged
merged 24 commits into from
Apr 3, 2024

Conversation

LuqiPan
Copy link
Contributor

@LuqiPan LuqiPan commented Mar 26, 2024

This PR will make reference docs for both endojs/endo and agoric/agoric-sdk repos available on docs.agoric.com.

Some finer prints:

  • to update the git hashes of repos we're generating docs from, run yarn git:update-submodules
  • deployment time is around 8 minutes vs 2 minutes prior to this change
  • I had to use a hack to make it build successfully on Cloudflare

Copy link

cloudflare-workers-and-pages bot commented Mar 26, 2024

Deploying documentation with  Cloudflare Pages  Cloudflare Pages

Latest commit: ba4232f
Status: ✅  Deploy successful!
Preview URL: https://97e1e3c9.documentation-7tp.pages.dev
Branch Preview URL: https://927-reference-docs-in-search.documentation-7tp.pages.dev

View logs

run: |
cd agoric-sdk
yarn add -D typedoc-plugin-markdown -W
yarn typedoc --plugin typedoc-plugin-markdown
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like this expects a yarn build before it.

packages/governance/test/swingsetTests/committeeBinary/test-committee.js:6:23 - error TS2307: Cannot find module '@agoric/zoe/bundles/bundle-contractFacet.js' or its corresponding type declarations.

6 import zcfBundle from '@agoric/zoe/bundles/bundle-contractFacet.js';

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder how much this slows down ci.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to be around 1 minute for the yarn build command

Copy link

Cloudflare Pages Preview URL: https://3c691980.documentation-7tp.pages.dev

Copy link

Cloudflare Pages Preview URL: https://40e7b3f7.documentation-7tp.pages.dev

@LuqiPan LuqiPan marked this pull request as ready for review March 27, 2024 22:04
# The reason we can no longer use Cloudflare's git integration to deploy docs
# site anymore is that Cloudflare's git integration would only check out
# documentation repo, whereas we need to check out both `endojs/endo` and
# `agoric/agoric-sdk` repo to build the docs site.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder about using submodules.

It would be nice to be able to get snippets from dapp-agoric-basics, for example.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I considered it but in my past experience I've only been frustrated about needing to update the hashes for submodules and make git commits. That said, there's not really a good solution whenever we are "orchestrating" multiple repos.

I'm happy to give it another try if there's some submodule-fu I'm missing though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe the submodule friction is a good thing?

Looks like this workflow is checking out HEAD of the other two repos, but that's not what people can use. I propose that the docs reflect what's in Mainnet. To do that requires a PR on this repo to specify the the branch or tag in the other repos. Expressing that as a git submodule makes sense and it sounds like it would obviate the need to define this additional workflow.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I propose that the docs reflect what's in Mainnet.

There's some appeal to that, but then how to we make progress on generated reference docs?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reference docs from each repo can publish anywhere. But this PR is about docs.agoric.com, right? Shouldn't that URL only show you what's in Mainnet?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

again, yes, there's some appeal to that, but running typedoc on the mainnet version of our code yields mostly garbage. So should the generated reference docs on dogs.agoric.com be garbage until our fixes to the jsdoc in agoric-sdk make it to mainnet?

Copy link
Member

@dckc dckc Mar 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Running typedoc on the mainnet version of the code won't work at all, in fact, will it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So should the generated reference docs on dogs.agoric.com be garbage until our fixes to the jsdoc in agoric-sdk make it to mainnet?

I'm not proposing that. I'm speaking to whether this repo sources from HEAD or a git submodule. My claim is that sourcing HEAD of agoric-sdk is untenable and so it shouldn't be how this PR is implemented. IIUC, there's no need for a new workflow if the reop used git submodules. Given that we need some way to specify a tag of the source repos, I propose that be how this is implemented.

What tag/version of the other repos to source, I think can change over time. I agree that u13 would be unworkable. Probably same for u14. But maybe u15 can get the docs improvements that have been going into master. Again, independent of my request that the repos are sourced by a tag instead of HEAD.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that u13 would be unworkable. Probably same for u14. But maybe u15 can get the docs improvements that have been going into master.

ok. yes, that's pretty much what I'm thinking.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This discussion is helpful, I'll look into submodules

package.json Outdated
@@ -7,7 +7,7 @@
"docs:dev": "NODE_OPTIONS=--openssl-legacy-provider vitepress dev main",
"docs:build": "NODE_OPTIONS=--openssl-legacy-provider vitepress build main",
"docs:preview": "NODE_OPTIONS=--openssl-legacy-provider vitepress preview main",
"docs:build-cf": "NODE_OPTIONS=--openssl-legacy-provider vitepress build main && cp _redirects dist/",
"docs:build-cf": "DEBUG='vitepress:*' NODE_OPTIONS=--openssl-legacy-provider vitepress build main && cp _redirects dist/",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The DEBUG environment variable would allow us to see what broke during a github workflow run.

We could potentially add this environment variable to other docs:* yarn commands too

@@ -83,6 +83,11 @@ export default defineConfig({
ignoreDeadLinks: [
// ignore all localhost links
/^https?:\/\/localhost/,
// ignore links that starts with ./packages/*
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some of the .md files generated from endo and agoric-sdk repo use these links below, which will fail the vitepress build as they're indeed dead links. There are 8 total offenders so it's not widespread, and I'm adding all of them here so they can be ignored by vitepress

turadg
turadg previously requested changes Mar 28, 2024
Copy link
Member

@turadg turadg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we will lose the lovely cloudflare-pages bot and its handy comment and links

That is a big loss. I request that how to stick with Cloudflare Pages be further studied.

I've only been frustrated about needing to update the hashes for submodules and make git commits.

How else will would you source a particular state of the other repos?

@dckc
Copy link
Member

dckc commented Mar 29, 2024

we will lose the lovely cloudflare-pages bot and its handy comment and links

That is a big loss.

We seem to still get comments with preview links: #1038 (comment)

It seems to only provide per-commit preview links and not per-branch links that update with new commits. I'll miss that. But I think I can deal.

Copy link
Member

@dckc dckc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hesitate to approve this when the only way to get to the generated docs is search (#1041)

@LuqiPan
Copy link
Contributor Author

LuqiPan commented Apr 1, 2024

Update:

We're going to submodules and back to build with Cloudflare (and thus Cloudflare Pages bot is working again), PR description updated too

@LuqiPan LuqiPan requested review from turadg and dckc April 1, 2024 20:28
Copy link
Member

@dckc dckc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to preview this locally, but I'm struggling to get it to work...

package.json Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
package.json Show resolved Hide resolved
issue_number: context.issue.number,
owner: context.repo.owner,
repo: context.repo.repo,
body: 'Cloudflare Pages Preview URL: ${{ steps.publish-to-cloudflare-pages.outputs.deployment-url }}'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a supported option to get a link based on a branch? one where the content is updated as commits are added to the branch, without changing the url

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not with wrangler-action: https://github.com/cloudflare/wrangler-action/blob/fd98a7c99091f581101a9a9bc7300a005d2f63e7/action.yml#L48-L53

Cloudflare Page bot is working again though so we retain the branch preview URL behavior

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah. .DISABLED

Copy link
Member

@dckc dckc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The resulting API docs need a lot of work, but @sufyaankhan is OK with landing this first.

For what it does, I think it looks good.

@LuqiPan LuqiPan dismissed turadg’s stale review April 3, 2024 03:39

Cloudflare Pages bot is working again and we're using submodules now. Let me know if you have additional comments, but for now I'm optimizing for getting this PR merged in.

@LuqiPan LuqiPan merged commit 81bbd1e into main Apr 3, 2024
4 checks passed
@LuqiPan LuqiPan deleted the 927-reference-docs-in-search branch April 3, 2024 03:40
@turadg
Copy link
Member

turadg commented Apr 3, 2024

Cloudflare Pages bot is working again and we're using submodules now. Let me know if you have additional comments, but for now I'm optimizing for getting this PR merged in.

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants