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

Replace gitbook with mdbook #187

Merged
merged 16 commits into from Oct 15, 2018
Merged

Replace gitbook with mdbook #187

merged 16 commits into from Oct 15, 2018

Conversation

@badboy
Copy link
Member

badboy commented Sep 19, 2018

This replaces gitbook with mdbook.

It currently uses the mdbook-dtmo wrapper, which bundles plugins for mermaid and ToC. Binary releases for Linux, Mac and Windows are provided on the release page (though Windows untested).
It's based on the latest git version of mdbook, which includes relevant bug fixes. I intend to update it to a stable released version of mdbook as soon as it gets a release.

Upstreaming/changing how we do the mermaid/toc processing is tracked in #186.

Known remaining issues:

  • I had to add empty chapters for some parts, see commit ffdc0b0
  • I had to remove the top-level "Bug template" link and moved it to the "Contributing" chapter

Live view: https://badboy.github.io/firefox-data-docs/
Fixes #162, #149

I can hold this back until the currently open PRs are merged and rebase it on top of that or I can help with rebasing the PRs after the fact.

@badboy badboy requested a review from harterrt Sep 19, 2018
Copy link
Collaborator

harterrt left a comment

Woot! Looks good to me with two small issues inline. Thanks so much for making this change @badboy!

Let's announce this change before merging. I'd like to give everyone a chance to poke around the newly rendered documentation to identify any issues. Do you have a fork hosted somewhere? If not, I have one set up here: https://harterrt.github.io/firefox-data-docs/

you'll need to install the `mdbook-dtmo` wrapper.
Binary builds are provided at <https://github.com/badboy/mdbook-dtmo/releases>.

You can install a binary build directly:

```bash

This comment has been minimized.

Copy link
@harterrt

harterrt Sep 19, 2018

Collaborator

I get an error:

install.sh: ERROR need rustc (command not found)

This comment has been minimized.

Copy link
@harterrt

harterrt Sep 19, 2018

Collaborator

Please note the rustc dependency. Once I fixed this dependency the install proceeded successfully. However, I didn't have cargo configured so ~/.cargo/bin was not in my $PATH and I got a mdbook-dtmo not found. Please also clarify this nit.

@@ -1,9 +1,9 @@
#!/bin/sh

build_dir='_book'
builddir='book'

This comment has been minimized.

Copy link
@harterrt

harterrt Sep 19, 2018

Collaborator

dropped an underscore here without updating line 6

This comment has been minimized.

Copy link
@harterrt

harterrt Sep 19, 2018

Collaborator

And lines 7, 11

@badboy

This comment has been minimized.

Copy link
Member Author

badboy commented Sep 19, 2018

Yeah, live view is linked: https://badboy.github.io/firefox-data-docs/
I wonder about the rustc thing, because in theory that should not be necessary, but I'll double-check. I will add the additional deps though.

What's the right place to announce this?

@harterrt

This comment has been minimized.

Copy link
Collaborator

harterrt commented Sep 19, 2018

Huh, I don't know where to announce this. @mreid-moz any ideas? I want to at least hit the folks reporting up through matt_g, rweiss, and kparlante.

Let's give the recipients a grace period by which to get straggling PRs submitted. Say 1week? Does that sound like a problem @badboy ?

@badboy

This comment has been minimized.

Copy link
Member Author

badboy commented Sep 19, 2018

Nope, timing is less critical for me. I have a script to do most of the changes automatically, so I can easily work of whatever is merged by then.

@harterrt

This comment has been minimized.

Copy link
Collaborator

harterrt commented Sep 24, 2018

Let's announce to fx-data-platform to start. SG, @badboy?

@mreid-moz

This comment has been minimized.

Copy link
Contributor

mreid-moz commented Sep 24, 2018

fx-data-platform sounds good to me. fx-data-dev would be good too, it is a bit broader (and public) and may find more people who are interested.

@badboy

This comment has been minimized.

Copy link
Member Author

badboy commented Sep 25, 2018

Will take care of fixing the above mentioned things and then also send a mail out.

@badboy

This comment has been minimized.

Copy link
Member Author

badboy commented Sep 28, 2018

I updated the code with the comments above.
IMO it would be fine to merge this by October 8th, giving a full week for gathering feedback. Anything that's merged until then will be migrated as well.

@harterrt With your ok I would send out a mail to fx-data-dev and fx-data-platform with this content: https://gist.github.com/badboy/2733a5c1a6c253c681200a5711631edd

@harterrt

This comment has been minimized.

Copy link
Collaborator

harterrt commented Sep 28, 2018

Sounds like a plan, @badboy. That email looks great, go for it!

@chutten

This comment has been minimized.

Copy link
Contributor

chutten commented Oct 1, 2018

The diagram for valid submissions isn't showing for me, even after reload.

@badboy

This comment has been minimized.

Copy link
Member Author

badboy commented Oct 2, 2018

@chutten The generated SVG by mermaid.js is empty. I'm looking into this.

@badboy

This comment has been minimized.

Copy link
Member Author

badboy commented Oct 2, 2018

Found the problem. Fixing and redeploying.

@mreid-moz

This comment has been minimized.

Copy link
Contributor

mreid-moz commented Oct 5, 2018

Some of the heading links on the landing page are broken (for example the "Tools" and "Cookbooks" links). For example, current Tools link is https://docs.telemetry.mozilla.org/tools/, in the new version it is https://badboy.github.io/firefox-data-docs/tools/README.html which 404s.

@badboy

This comment has been minimized.

Copy link
Member Author

badboy commented Oct 8, 2018

Thanks, @mreid-moz. I fixed those links and also ran a link checker locally to ensure there are no broken links hiding. I rebased on top of the latest master changes and ensure the new docu is rendering fine as well.

@badboy

This comment has been minimized.

Copy link
Member Author

badboy commented Oct 8, 2018

I had to use absolute URLs for included files. These are already broken in the current version. I added them to the ignore list of the link checker.

@chutten

This comment has been minimized.

Copy link
Contributor

chutten commented Oct 9, 2018

"Running a link checker" sounds like either a good checker for the repo or a decent thing to document in README or CONTRIBUTING or someplace.

@mreid-moz

This comment has been minimized.

Copy link
Contributor

mreid-moz commented Oct 9, 2018

I had to use absolute URLs for included files. These are already broken in the current version. I added them to the ignore list of the link checker.

Does this mean we won't have link-checking for internal links?

@mreid-moz

This comment has been minimized.

Copy link
Contributor

mreid-moz commented Oct 9, 2018

"Running a link checker" sounds like either a good checker for the repo or a decent thing to document in README or CONTRIBUTING or someplace.

It's documented here (or here in the "live" docs).

@badboy

This comment has been minimized.

Copy link
Member Author

badboy commented Oct 9, 2018

We do have the markdown-link-checker, which does work and mdbook translates links to markdown files into the appropriate links to the rendered html file.

However, we have some files that are included in different parts, that used relative links to markdown files. Those files in markdown are correct from the point of view of that single markdown file, but incorrect when rendered, as they are included elsewhere.
These links are already currently broken, e.g. the telemetry_new_profile_parquet link on https://docs.telemetry.mozilla.org/concepts/choosing_a_dataset.html#firstshutdownsummary

I can see if I can somehow add a link-checker on the actual rendered content to CI, though I hope this would not block this PR.

@badboy

This comment has been minimized.

Copy link
Member Author

badboy commented Oct 12, 2018

Just pushed the latest rebase to include changes to the project listing.

@harterrt From my side it would be ready to go. I would handle the link-checker thing in a follow-up, if that's ok.

Copy link
Collaborator

harterrt left a comment

Sounds like the link-checker work is fixing a bug that existed before making this change. I agree that should be marked as future work. Is there an issue filed?

LGTM!

@badboy

This comment has been minimized.

Copy link
Member Author

badboy commented Oct 15, 2018

@badboy badboy merged commit e2885d4 into mozilla:master Oct 15, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
mreid-moz added a commit that referenced this pull request Nov 12, 2018
Fix refs to GitBook and link to the contributing info that were changed in #187
@mreid-moz mreid-moz mentioned this pull request Nov 12, 2018
mreid-moz added a commit that referenced this pull request Nov 13, 2018
Fix refs to GitBook and link to the contributing info that were changed in #187
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.