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

Add mdbook and GitHub pages deployment #319

Merged
merged 1 commit into from
Aug 10, 2021
Merged

Add mdbook and GitHub pages deployment #319

merged 1 commit into from
Aug 10, 2021

Conversation

XAMPPRocky
Copy link
Collaborator

This PR adds mdbook for documentation and adds a GitHub Action to deploy both it and the API documentation to GitHub pages, so we'll always have the latest documentation available if needed.

@markmandel You'll need to enable GitHub Pages in settings for this to work.

@XAMPPRocky XAMPPRocky requested review from markmandel and iffyio July 7, 2021 13:05
@google-cla google-cla bot added the cla: yes label Jul 7, 2021
@quilkin-bot

This comment has been minimized.

@XAMPPRocky XAMPPRocky added this to the Release milestone Jul 7, 2021
@markmandel
Copy link
Member

Can we merge this after release? Otherwise we have to review all our docs all over again, make sure our quickstarts still work etc.

It's just a big sweeping change when we have limited time.

@markmandel
Copy link
Member

Another question on this format - if changes from main are auto pushed to the github page, does that mean docs that have changed due to refactoring/new feature/etc on main go straight to what is live, and will therefore be incorrect?

If I'm an end user is there an easy way for me to just see documentation for the release version?

docs/book.toml Outdated Show resolved Hide resolved
Copy link
Member

@markmandel markmandel left a comment

Choose a reason for hiding this comment

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

This will also need to change the documentation links on https://github.com/googleforgames/quilkin/blob/main/README.md, as if we merge this, then they all break.

@markmandel
Copy link
Member

Another thought - can we add what you need to install etc to run this locally to our development guide?
https://github.com/googleforgames/quilkin/blob/main/build/README.md

If we want to go super above and beyond a make docs in the build directory would be super cool.

@markmandel
Copy link
Member

Just checking gh-pages enablement. If you want to do a preview, I need there to be an existing branch. I had a quick peek, and there doesn't seem to be a gh-pages branch yet. Drop me a note when it's there, and I'll turn things on 👍🏻 (Maybe do it manually to get started?)

image

On review, I'm less concerned with this PR before release - just want to make sure that none of our links to documentation break.

Copy link
Member

@markmandel markmandel left a comment

Choose a reason for hiding this comment

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

Just digging into mdbook a bunch, and I have lots of questions 😄 - it's super cool though! I especially love the idea of being able include rust files directly, since we can tie that to CI. Excellent suggestion to move us to mdbook!

  • Does this also build the rust API docs and push them to github pages, or just the pages in docs/src ? (I don't think it does the rust api docs, but wanted to be sure). If not, should we?
  • I'd love to see this also include the work to integrate mdbook test into CI, so that we know everything passes on each PR run (unless you are going to do this in a follow up PR? -- we should probably ticket this work then).
  • Related to above, re: overwriting github pages with each post to main -- can we make snapshots for each release? It would be nice if main points to something like /develop and each release is a /0.1.0/, and /0.2.0/ directory etc. Since the API is likely to change a bunch, I expect it will be useful for people to be able to go back in time and see how the documentation changes / or check the documentation for the release they are currently using.

Not sure if it 100% solves the initial broken docs on merge issues, but one other thought I had was to point the documentation links in the README.md to the release branch for the time being, until we feel very confident in this new platform, and we can then make a switch over. Just an idea though.

.github/workflows/docs.yml Show resolved Hide resolved
@XAMPPRocky
Copy link
Collaborator Author

XAMPPRocky commented Jul 12, 2021

Just checking gh-pages enablement. If you want to do a preview, I need there to be an existing branch. I had a quick peek, and there doesn't seem to be a gh-pages branch yet

I forgot that's how that works, then you'll just need to enable it after this has merged then. The branch will be created on the first successful run.

Does this also build the rust API docs and push them to github pages, or just the pages in docs/src ? (I don't think it does the rust api docs, but wanted to be sure). If not, should we?

This does also build the Rust API docs and publishes them under /api (book is under /book ["book is book"]), because @iffyio mentioned being concerned about updated the API docs not being available between releases in #293. I have no preference whether we do or not.

I'd love to see this also include the work to integrate mdbook test into CI,

Related to above, re: overwriting github pages with each post to main -- can we make snapshots for each release?

I can add those.

@XAMPPRocky XAMPPRocky self-assigned this Jul 12, 2021
@XAMPPRocky XAMPPRocky force-pushed the deploy-docs branch 3 times, most recently from a1e4312 to 4651ca6 Compare July 12, 2021 12:52
@XAMPPRocky
Copy link
Collaborator Author

Added snapshotting. So now this runs when we've pushed or tagged a release with v* e.g. v0.1.0, and will deploy the documentation to the respective directory. Let's make adding make test support a follow up item because right now I can't build with make test on my machine, ld runs out of space before it can finish linking.

@quilkin-bot

This comment has been minimized.

@quilkin-bot

This comment has been minimized.

@quilkin-bot

This comment has been minimized.

1 similar comment
@quilkin-bot

This comment has been minimized.

@quilkin-bot
Copy link
Collaborator

Build Succeeded 🥳

Build Id: 9bb613f9-1d0f-451e-98a5-12c199c59feb

To build this version:

git fetch [email protected]:googleforgames/quilkin.git pull/319/head:pr_319 && git checkout pr_319
cargo build

@XAMPPRocky XAMPPRocky requested a review from markmandel July 12, 2021 13:23
@XAMPPRocky XAMPPRocky removed this from the Release milestone Jul 12, 2021
@markmandel
Copy link
Member

Once we are done with release, I'm going to try and run this on a personal fork (I believe we can do that with github actions?) so we can have a live previous of what this looks like and triple check everything is working as expected.

This is going to be nice though - I can see how this is going to setup lots of nice improvements in the documentation. 😄

@markmandel
Copy link
Member

Ran it again, ran into a stack of issues unfortunately. Have fixed several things here:
https://github.com/markmandel/quilkin/blob/pr/deploy-docs/.github/workflows/docs.yml

(Ignore where I have pr/** as a push branch, that's just for testing).

  • Setting of env variables (docs) didn't pass between steps, so switched that out.
  • Fixed issue with the snapshot variable so it can handle branch names with "/" in them
  • Fix issues with mkdir for the same thing.

Successful run:
https://github.com/markmandel/quilkin/runs/3139428581?check_suite_focus=true

I learnt some things about github actions though!

Preview:
https://markmandel.github.io/quilkin/pr/deploy-docs/book/
https://markmandel.github.io/quilkin/pr/deploy-docs/api/quilkin/

I haven't had a chance to review the docs yet, but at least we can now see them 🙌🏻

I also haven't tested it against creating a tag, so one of us should also take that for a spin so we can make sure it works as expected. That will likely be my next step unless you get to it first.

@markmandel
Copy link
Member

Tagging seems to be working as expected (as long as the action are in the tagged content).

I cleaned up the GH pages on my fork, and manually made a snapshot for 0.1.0 - once we resolve the below blocking issues, and if we're happy, I'll push it to main Quilkin repo, and make a PR to point the README.md to it, and then we can get this merged with my fixes. (TIL you can't make a PR to a branch that doesn't exist!)

0.1.0 documentation (guide, api)

The blocking issues I found were:

  • The documentation for Filters is missing from the TOC.
  • Let's add an examples page that points to https://github.com/googleforgames/quilkin/blob/main/examples (I'd love to have it point to the same release branch as the snapshot with something like mdbook-variables, but can experiment with that later, I'm sure an explanatory note that they might need to switch to the correct release tag will in the repo will suffice).

I am keen to get this merged before we change the API surface, as otherwise it becomes quite difficult to get a mdbook snapshot of 0.1.0 (which we are still inline with at this point).

The other option is: We skip a snapshot of 0.1.0, and instead point the documentaton for 0.1.0 straight at https://github.com/googleforgames/quilkin/blob/release-0.1.0/README.md, and then 0.2.0 will be the first documentation snapshot we take with mdbook.

Thoughts?

markmandel added a commit to markmandel/quilkin that referenced this pull request Jul 26, 2021
Break out a section on the README.md linking to tagged snapshots of
documentation, so that as API changes occur it's still easy to see
previous API documentation and accompanying guides. It also stops new
users from getting confused by in-flight API changes.

I believe this will aso unblock googleforgames#293 and also remove the requirement to
get a 0.1.0 snapshot for googleforgames#319, which could block other PRs as well.
markmandel added a commit to markmandel/quilkin that referenced this pull request Jul 26, 2021
Break out a section on the README.md linking to tagged snapshots of
documentation, so that as API changes occur it's still easy to see
previous API documentation and accompanying guides. It also stops new
users from getting confused by in-flight API changes.

I believe this will aso unblock googleforgames#293 and also remove the requirement to
get a 0.1.0 snapshot for googleforgames#319, which could block other PRs as well.
markmandel added a commit to markmandel/quilkin that referenced this pull request Jul 26, 2021
Break out a section on the README.md linking to tagged snapshots of
documentation, so that as API changes occur it's still easy to see
previous API documentation and accompanying guides. It also stops new
users from getting confused by in-flight API changes.

I believe this will aso unblock googleforgames#293 and also remove the requirement to
get a 0.1.0 snapshot for googleforgames#319, which could block other PRs as well.
markmandel added a commit to markmandel/quilkin that referenced this pull request Jul 26, 2021
Break out a section on the README.md linking to tagged snapshots of
documentation, so that as API changes occur it's still easy to see
previous API documentation and accompanying guides. It also stops new
users from getting confused by in-flight API changes.

I believe this will aso unblock googleforgames#293 and also remove the requirement to
get a 0.1.0 snapshot for googleforgames#319, which could block other PRs as well.
@quilkin-bot
Copy link
Collaborator

Build Succeeded 🥳

Build Id: 046bb167-2429-40f3-8883-c69cad97ac72

To build this version:

git fetch [email protected]:googleforgames/quilkin.git pull/319/head:pr_319 && git checkout pr_319
cargo build

@XAMPPRocky XAMPPRocky force-pushed the deploy-docs branch 2 times, most recently from 3b84fd6 to 0345602 Compare August 2, 2021 12:00
@quilkin-bot
Copy link
Collaborator

Build Failed 😭

Build Id: e1ff381d-4a8f-4c97-be62-eb6c1d20c0c1

Status: FAILURE

To get permission to view the Cloud Build view, join the quilkin-discuss Google Group.

@quilkin-bot
Copy link
Collaborator

Build Failed 😭

Build Id: fa8327bd-fdc4-48f6-a85c-505e449494fc

Status: FAILURE

To get permission to view the Cloud Build view, join the quilkin-discuss Google Group.

@XAMPPRocky
Copy link
Collaborator Author

Let's add an examples page that points to https://github.com/googleforgames/quilkin/blob/main/examples (I'd love to have it point to the same release branch as the snapshot with something like mdbook-variables, but can experiment with that later, I'm sure an explanatory note that they might need to switch to the correct release tag will in the repo will suffice).

Well you can't link to it directly in the TOC, and we already link to that page in the introduction, so if we want a more detailed page, I think that should come as a separate PR.

I've implemented the rest of the feedback.

@quilkin-bot
Copy link
Collaborator

Build Succeeded 🥳

Build Id: 8f0ff355-4667-4f09-94f9-8d08afb83a1c

To build this version:

git fetch [email protected]:googleforgames/quilkin.git pull/319/head:pr_319 && git checkout pr_319
cargo build

Copy link
Member

@markmandel markmandel left a comment

Choose a reason for hiding this comment

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

Let's add an examples page that points to https://github.com/googleforgames/quilkin/blob/main/examples (I'd love to have it point to the same release branch as the snapshot with something like mdbook-variables, but can experiment with that later, I'm sure an explanatory note that they might need to switch to the correct release tag will in the repo will suffice).

Well you can't link to it directly in the TOC, and we already link to that page in the introduction, so if we want a more detailed page, I think that should come as a separate PR.

Sounds good, I'll jump on it once this has merged 👍🏻

(Reminder to myself: Update the release guide once this has merged too, to link to the tagged snapshot from README.md)

.github/workflows/docs.yml Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@quilkin-bot
Copy link
Collaborator

Build Succeeded 🥳

Build Id: 8a19e188-70a0-4b8d-99b3-d8091d75c5ec

To build this version:

git fetch [email protected]:googleforgames/quilkin.git pull/319/head:pr_319 && git checkout pr_319
cargo build

@quilkin-bot
Copy link
Collaborator

Build Succeeded 🥳

Build Id: bb76fe42-95ab-47bb-a6d3-e4c0ffd1b86e

To build this version:

git fetch [email protected]:googleforgames/quilkin.git pull/319/head:pr_319 && git checkout pr_319
cargo build

Copy link
Member

@markmandel markmandel left a comment

Choose a reason for hiding this comment

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

YAY!

@quilkin-bot
Copy link
Collaborator

Build Succeeded 🥳

Build Id: 3a75105e-b8b4-4f20-bf8a-0d919f324986

To build this version:

git fetch [email protected]:googleforgames/quilkin.git pull/319/head:pr_319 && git checkout pr_319
cargo build

@markmandel markmandel merged commit c187b5a into main Aug 10, 2021
@markmandel markmandel deleted the deploy-docs branch August 10, 2021 00:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants