Set up documentation website generation and deployment #253

japborst · 2022-09-21T08:32:21Z

Scope
The scope of this PR is to bootstrap a documentation website.

Readme how to run/use the Jekyll website
GitHub action to deploy automatically upon merge to master
Has a homepage (identical to README.md from GitHub)

Follow-ups
We exclude auto-generating the docs, which will be done in a follow-up PR.

Lists all BugPattern checks
- Displays summary, description, severity, and tag (currently dummy values)
- Displays source code link
- In-line replacement examples
Lists all Refaster templates (grouped per collection)
- In-line replacement examples
Support additional Markdown docs

Preview

japborst · 2022-09-21T09:34:13Z

.github/workflows/deploy-website.yaml

+    #branches: [$default-branch]
+    branches:
+      - 'sschroevers/deploy-to-github-pages'


Before merging, this needs to be reverted.

japborst · 2022-09-21T09:34:37Z

.github/workflows/deploy-website.yaml

+      - name: Generate documentation
+        run:  bash ./generate-docs.sh


Everything is standard in this workflow, except for running the doc generation

japborst · 2022-09-21T09:35:36Z

README.md

-  <source media="(prefers-color-scheme: light)" srcset="logo.svg">
-  <img alt="Error Prone Support logo" src="logo.svg" width="50%">
-</picture>
+  <picture>


A pity, but when using the readme for our website, you can't mix HTML and Markdown. That is normal behaviour, but apparently was okay on GitHub's flavoured Markdown before.

japborst · 2022-09-21T09:36:09Z

docs/bugpatterns/EmptyMethod.md

@@ -0,0 +1 @@
+There's not much use to keep empty methods.


Used as an example for additional docs on a bugpattern. We should improve this, or use another example.

Stephan202 · 2022-09-22T08:13:14Z

generate-docs.sh

+REFASTER_FOLDER="${WEBSITE_FOLDER}/refastertemplates"
+REFASTER_DOCS_FOLDER="${DOCS_FOLDER}/refastertemplates"


So IIUC the idea right now is to have one top-level page for each refaster template collection. It would be nice then to enumerate the constituent templates on each pages, accessible through a URL anchor. See the example URL in #255.

Indeed. It's intended to list the template collections per page, instead of a page per template. @oxkitsune is working on retrieving and exposing the before/after templates.

rickie

Added a commit :).

rickie · 2022-09-27T13:42:46Z

website/.gitignore

@@ -0,0 +1,10 @@
+# Jekyll generated files


Do we want these entries here or in the root .gitignore?

Used www.gitignore.io to add some more dirs and files related to Jekyll.

Would personally put here, easier to manage and scoped to the website. Where the root gitignore is more focussed on the Java development

rickie · 2022-09-27T13:55:44Z

website/_config.yml

+exclude:
+  - Gemfile
+  - Gemfile.lock
+  - README.md


I had to add vendor here because I got an issue while generating. Related GH thread.

Interesting... I did not have that issue 🤔

I don't have this issue either. And I also don't see the .bundle and vendor directories mentioned in the .gitignore 🤔

rickie · 2022-09-27T14:25:15Z

(Oops, did it on gdejong/docgen, will move it ~later)

japborst · 2022-09-27T15:10:33Z

README.md

+  <picture>
+    <source media="(prefers-color-scheme: dark)" srcset="website/assets/images/logo-dark.svg">
+    <source media="(prefers-color-scheme: light)" srcset="website/assets/images/logo.svg">
+    <img alt="Error Prone Support logo" src="website/assets/images/logo.svg" width="50%">


Changed the path to the website assets for the logo, s.t. we only store it in 1 place,

japborst · 2022-09-27T15:12:53Z

As the documentation generation will be done in the scope of https://github.com/PicnicSupermarket/error-prone-support/tree/gdejong/docgen, I removed it from this PR.

That means this PR focusses just on bootstrapping the website, and is now ready for your review @rickie @Stephan202.

website/_config.yml

rickie

Added a commit with some changes. Feel free to tweak them.
The README nicely explains how to start working on the website as well. Used it to set it up locally and works like a charm.

This is such a cool setup! I really like how the website looks and what we can do with it ❤️!

I'm following this ticket for when we can use a title + logo in the top left corner haha.

Thanks for all the effort 🚀 !!!

Suggested commit message:

Set up documentation website generation and deployment (#253)

Generating the website is done by Jekyll with a theme called `just-the-docs`. 
Deployment of the website is done via GitHub Pages.

See:
- https://github.com/jekyll/jekyll
- https://github.com/just-the-docs/just-the-docs

.github/workflows/build.yaml

rickie · 2022-09-28T17:58:23Z

website/_config.yml

+description: >-
+  Error Prone extensions: extra bug checkers and a large battery of Refaster templates.
+
+remote_theme: just-the-docs/just-the-docs


Shouldn't we pin the version? As described here.

Not looking to pin to an older version, as the newer version has some improvements I'd like to have included. We can pin, but does increase maintenance of upgrading. I'm fine with both directions here.

Looks like we can use the Gemfile instead. The only difference I see locally is that with v0.4.0.rc2 the "quote" in the readme doesn't have a grey bar on the left hand side. I guess that's only fixed in a non-released version.

Since using the Gemfile (a) reduces maintenance (assuming we set up Renovate) and (b) obviates the need for jekyll-remote-theme: let's go with that.

Okay, the build now fails with Could not find 'just-the-docs' (>= 0) among 148 total gem(s) (Gem::MissingSpecError). I guess there's a difference between my local and the GHA setup. To be investigated.

IIUC it happens because this image doesn't run bundle install. That would also mean that the Gemfile contents are misleading. This comment suggests an alternative approach. Not sure I'll get around to trying this today still, though...

Most actions run in the scope of a Docker image IIUC, so that means indeed bundle install of our own gemfile isn't run 😞

.github/workflows/deploy-website.yaml

rickie · 2022-09-28T18:05:23Z

.github/workflows/deploy-website.yaml

+          destination: ./_site
+      - name: Upload website artifact
+        uses: actions/upload-pages-artifact@v1.0.3
+  deploy:


I think it would be clearer to have a newline between these two parts.

Indentation already takes care of that. We also don't do that above, or in build.yaml.

generate-docs.sh

rickie · 2022-09-28T18:06:21Z

generate-docs.sh

+    sed $SEDOPTION 's/srcset="website\//srcset="/g' ${HOMEPAGE}
+}
+
+# Do it


We can either remove this comment or say something like:

# Start generating the website.

website/_config.yml

website/_includes/head_custom.html

website/_sass/custom/custom.scss

rickie · 2022-09-28T18:30:48Z

Rebased.

japborst · 2022-09-28T20:21:32Z

Thanks @rickie. I tweaked the suggestions a little.

Let's go 🎸

rickie

Nice, tweaks look good to me!

.github/workflows/build.yaml

japborst · 2022-10-01T06:07:37Z

I added two things, which I believe were important to do for the website

Auto-generate a sitemap.xml / robots.txt for SEO and scaping
Introduce html-proofer to validate all our URLs (also used in https://github.com/PicnicSupermarket/picnicsupermarket.github.io)

The latter actually caught quite some invalid urls in our readme, as relative paths won't work when hosting our site.

FYI @rickie as this was added after your review

japborst · 2022-10-01T06:08:51Z

.github/workflows/deploy-website.yaml

+      - name: Validating HTML output
+        uses: athackst/htmlproofer-action@main
+        with:
+          check_external_hash: false


In some websites a hash is used for JS routing, which is done after loading. Disabling this to reduce false-positives.

japborst · 2022-10-01T06:09:16Z

README.md

@@ -223,5 +221,5 @@ guidelines][contributing].
 [pitest-maven]: https://pitest.org/quickstart/maven
 [pr-badge]: https://img.shields.io/badge/PRs-welcome-brightgreen.svg
 [refaster]: https://errorprone.info/docs/refaster
-[refaster-templates-bigdecimal]: error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/BigDecimalTemplates.java
-[refaster-templates]: error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/
+[refaster-templates-bigdecimal]: https://github.com/PicnicSupermarket/error-prone-support/blob/master/error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/BigDecimalTemplates.java


Absolute paths are required, as the same page is used to service our website.

Clear! (I should probably just test this, but) will the build fail if we accidentally reintroduce a relative URL?

Exactly why I introduced html-proofer s.t. we catch this stuff (didn't know the existence before, but came across in another repo and a very obvious check to introduce).

Certainly! (And we shouldn't stop there. I made a note to also introduce shellcheck. We should likely enable other kind of static analysis.)

japborst · 2022-10-01T07:40:46Z

Open question @Stephan202 (?)

Should we build on all branches, and only deploy on the default branch? Would allow verifying things work (e.g. html proofer and page generation).

Stephan202

Rebased and added a commit. Very nice work @japborst!

I'm not quite done reviewing, but thought I'd flush the changes and questions I have so far.

Stephan202 · 2022-10-02T11:56:17Z

website/Gemfile

+source "https://rubygems.org"
+gem "github-pages", "~> 227"
+gem "rake", "~> 13.0" # Required for "just-the-docs" theme.
+gem "jekyll-sitemap", "~> 1.4"
+gem "html-proofer", "~> 4.4"


@Badbond now we should also consider enabling Renovate support for Gemfiles 😄

^ Since I love maximally reproducible builds, I propose to pin the exact versions. I.c.w. Renovate that shouldn't be an issue.

Supporting Renovate here would also allow upgrading gemfiles in other codebases, like iOS.

Created internal ticket to track this 👍.

Stephan202 · 2022-10-02T14:53:58Z

website/_config.yml

+  - README.md
+  - vendor
+
+permalink: pretty # Use /doc/ instead of /doc.html.


In general it does a bit more than that. (But I can see how the date components are omitted in this case.)

Stephan202 · 2022-10-02T14:57:16Z

website/_config.yml

+# Theme configuration.
+search_enabled: true
+heading_anchors: true


These are the default values, IIUC. We generally omit defaults; would suggest we do the same here.

Stephan202 · 2022-10-02T15:00:52Z

website/_config.yml

+plugins:
+- jekyll-remote-theme
+- jekyll-sitemap


Upon further reading, it seems that while we don't need to specify these in Gemfile, doing so would improve control over the build.

Only for jekyll-remote-theme. We still do need to specify jekyll-sitemap.

Current code work without it. (That said, I should have dropped this comment before flushing: we unconditionally depend on many jekyll-* gems, so singling out jekyll-sitemap doesn't make much sense.)

Stephan202 · 2022-10-02T15:11:33Z

website/_config.yml

+description: >-
+  Error Prone extensions: extra bug checkers and a large battery of Refaster templates.
+
+remote_theme: just-the-docs/just-the-docs


Looks like we can use the Gemfile instead. The only difference I see locally is that with v0.4.0.rc2 the "quote" in the readme doesn't have a grey bar on the left hand side. I guess that's only fixed in a non-released version.

Since using the Gemfile (a) reduces maintenance (assuming we set up Renovate) and (b) obviates the need for jekyll-remote-theme: let's go with that.

website/_sass/custom/custom.scss

Stephan202 · 2022-10-02T15:49:04Z

website/_sass/custom/custom.scss

+// We should override $nav-width(-md), however this breaks code highlighting and other styles.
+// This seems to be an issue for following the recommended approach.
+// See https://github.com/just-the-docs/just-the-docs/issues/982.


Nice that you filed a bug report! I see that the issue will be fixed in the next release. When we upgrade concrete change should we make instead? (Sorry for the n00b question, I know little about this stuff.)

No, this will continue to work. When that bug gets fixed, we can adhere to the official/recommended approach of creating a custom scheme and overriding $nav-width s.t. that param gets used throughout the rest of the styling.

See https://just-the-docs.github.io/just-the-docs/docs/customization/#custom-schemes

Check. Didn't fully grok that page yet, but pushed a commit with a proposal :)

Stephan202 · 2022-10-02T15:55:06Z

website/assets/images/site.webmanifest

+{
+    "name": "",
+    "short_name": "",
+    "icons": [
+        {
+            "src": "/assets/images/android-chrome-192x192.png",
+            "sizes": "192x192",
+            "type": "image/png"
+        },
+        {
+            "src": "/assets/images/android-chrome-512x512.png",
+            "sizes": "512x512",
+            "type": "image/png"
+        }
+    ],
+    "theme_color": "#ffffff",
+    "background_color": "#ffffff",
+    "display": "standalone"
+}


I found this explanation. Excuse my ignorance; some questions:

In our case, what is the expected use case?

Should we file in the name and short_name?

Is this Chrome/Android-specific, or should we select more generic file names?

How were the dimensions chosen?

This too was generated using https://realfavicongenerator.net/ (mentioned here https://github.com/PicnicSupermarket/error-prone-support/blob/f62b632fba36c18510b9737f6720bd16c758e386/website/_includes/head_custom.html).

This is used when adding our docs to your homescreen. See https://realfavicongenerator.net/faq#manifest

Your point stands, we should set name and short name.

I did not want to add a comment at the top, as JSON comments are not widely supported and could break things.

Indeed, can't put a comment here.

Given the generic name of both the file and the JSON property (icons), it seems to me can use more generic image file names. (But perhaps I'm missing something; maybe one of the coming days I'll play with https://realfavicongenerator.net/ myself.)

Stephan202 · 2022-10-02T15:57:49Z

website/_includes/head_custom.html

+<link rel="icon" type="image/png" sizes="16x16" href="/assets/images/favicon-16x16.png">
+<link rel="manifest" href="/assets/images/site.webmanifest">
+<link rel="mask-icon" href="/assets/images/safari-pinned-tab.svg" color="#5bbad5">
+<link rel="shortcut icon" href="/favicon.ico">


I guess this one is not inside /assets for compatibility with old browers? Even if so: can't we configure Jekyll to put it in the root, without having it in the website root in the Github repo?

Yep, some (older) browsers assume it's in the root. Also, some scrapers make this assumption. See also https://realfavicongenerator.net/faq#why_icons_in_root

If we really don't want to have in the root, we could move it and disable the favicon check for html-proofer (as that also complains about it). However, I'd prefer to keep it in the root, as is common practice for compatibility.

Hmm, I mean "is there a way to convince Jekyll to put it into the website/_site root, without it being located in website. Googling around perhaps a hook would do it.

Personally, I'm not sure if that's any better. We're already in website. So you expect the files are reflected on what we're hosting. Putting something in assets/images, but then referring the root here where some webhook or script puts it there, makes development all the more confusing IMHO.

Ack, I'll not make a further point of it then.

Stephan202 · 2022-10-02T16:01:43Z

website/Gemfile

@@ -0,0 +1,5 @@
+source "https://rubygems.org"
+gem "github-pages", "~> 227"
+gem "rake", "~> 13.0" # Required for "just-the-docs" theme.


If we manage just-the-docs in this Gemfile, then we no longer need to specify rake 😄

website/_sass/custom/custom.scss

japborst · 2022-10-02T20:06:09Z

It seems that github-pages / jekyll-build-pages automatically ships with jekyll-remote-theme, which was introduce to use themes on GitHub that did not have a nice gem provided https://www.siteleaf.com/blog/remote-themes/

Whilst specifying the gem works locally, the build - however - doesn't actually run bundle install so it fails the build when we're using a local theme.

That leads two options:

Setup ruby, bundler, and run the install manually
Pin the remote theme for a reproducable build

I've now done the latter, as that drastically simplifies the build setup. As we're pinning the version and the latest is an RC, we should keep an eye out for the final v4 release.

japborst · 2022-10-03T07:06:13Z

Discussed offline with @Stephan202.

Pinning the version is okay for now, but we should investigate a 100% reproducible build s.t. the setup in GHA and locally are 1:1 the same. This is not just for the theme, but for everything (jekyll, html-proofer, etc.)

rickie

Added a commit.

Caught up on all the discussions, changes LGTM.

@maxsumrall Do you want to finish your review still 😄 ?

maxsumrall · 2022-10-03T12:19:41Z

Added a commit.

Caught up on all the discussions, changes LGTM.

@maxsumrall Do you want to finish your review still 😄 ?

Nope, just had the comment, but seems like a lot of effort to get that link to work.

nathankooij · 2022-10-03T13:05:52Z

website/Gemfile

@@ -0,0 +1,5 @@
+source "https://rubygems.org"
+gem "github-pages", "227"


Should we also fix the Ruby version used? See https://bundler.io/guides/gemfile_ruby.html#specifying-a-ruby-version.

Would like to make part of the larger initiative, see #253 (comment)

Sorry, I didn't read all of the comments so was just a drive-by comment. SGTM 👍

Stephan202

Added one more commit. Some of the filenames suggested by realfavicongenerator.net are still surprising to me, but let's roll with it.

Slightly tweaked the suggested commit message.

Major milestone 💪

Stephan202 · 2022-10-04T05:27:54Z

generate-docs.sh

@@ -0,0 +1,34 @@
+#!/usr/bin/env bash


I'll push some suggestions for this file. (Not relying on the side-effect of cd, addressing shellcheck output, etc.)

japborst · 2022-10-04T07:04:56Z

Yep, favicons are a mess 🙄 Just look at the list of "Why so many files?" at https://realfavicongenerator.net/faq.

Thanks for the last tweaks @Stephan202 🙏

japborst · 2022-10-04T07:18:05Z

ps. before merging I verified that generate-docs.sh also runs as expected on a MacOS machine (as it was authored on a Linux machine).

japborst requested a review from rickie September 21, 2022 08:32

japborst force-pushed the sschroevers/deploy-to-github-pages branch from e9e37c8 to ccf3ce4 Compare September 21, 2022 09:33

japborst commented Sep 21, 2022

View reviewed changes

japborst temporarily deployed to github-pages September 21, 2022 09:34 Inactive

japborst commented Sep 21, 2022

View reviewed changes

japborst temporarily deployed to github-pages September 21, 2022 09:51 Inactive

Stephan202 reviewed Sep 22, 2022

View reviewed changes

japborst temporarily deployed to github-pages September 24, 2022 11:44 Inactive

japborst temporarily deployed to github-pages September 27, 2022 08:21 Inactive

rickie reviewed Sep 27, 2022

View reviewed changes

japborst force-pushed the sschroevers/deploy-to-github-pages branch from 4736144 to 2708207 Compare September 27, 2022 15:09

japborst commented Sep 27, 2022

View reviewed changes

japborst marked this pull request as ready for review September 27, 2022 15:11

japborst commented Sep 28, 2022

View reviewed changes

website/_config.yml Show resolved Hide resolved

maxsumrall reviewed Sep 28, 2022

View reviewed changes

website/_config.yml Outdated Show resolved Hide resolved

japborst added this to the 0.4.0 milestone Sep 28, 2022

rickie added the new feature label Sep 28, 2022

rickie approved these changes Sep 28, 2022

View reviewed changes

rickie force-pushed the sschroevers/deploy-to-github-pages branch from 702b589 to db95b9c Compare September 28, 2022 18:30

rickie reviewed Sep 29, 2022

View reviewed changes

.github/workflows/build.yaml Outdated Show resolved Hide resolved

japborst commented Oct 1, 2022

View reviewed changes

japborst and others added 3 commits October 2, 2022 14:52

Attempt to force checking html

a0619f6

Use other proofer action

e2adb4a

Suggestions

d295e60

Stephan202 force-pushed the sschroevers/deploy-to-github-pages branch from 584251d to d295e60 Compare October 2, 2022 16:03

Stephan202 reviewed Oct 2, 2022

View reviewed changes

Suggestion

aa045b5

japborst commented Oct 2, 2022

View reviewed changes

website/_sass/custom/custom.scss Outdated Show resolved Hide resolved

japborst added 3 commits October 2, 2022 21:27

Tweaks

67e2f07

Use remote theme

ed33906

Tweaks

baee39a

japborst and others added 2 commits October 2, 2022 22:06

Update local dev setup for remote themes

db0dc5d

Suggestions

1b5220a

Suggestions

ade5782

rickie approved these changes Oct 3, 2022

View reviewed changes

nathankooij reviewed Oct 3, 2022

View reviewed changes

japborst mentioned this pull request Oct 3, 2022

Unify local and GitHub Actions website generation flow #274

Merged

Suggestions

5a0c65e

Stephan202 approved these changes Oct 4, 2022

View reviewed changes

japborst changed the title ~~Bootstrap documentation website~~ Set up documentation website generation and deployment Oct 4, 2022

japborst merged commit 5ca95eb into master Oct 4, 2022

japborst deleted the sschroevers/deploy-to-github-pages branch October 4, 2022 07:08

japborst mentioned this pull request Oct 5, 2022

Fix default branch reference in GitHub Actions workflow definitions #278

Merged

rickie added improvement and removed new feature labels Oct 13, 2022

Badbond mentioned this pull request Dec 12, 2022

Update website styling and add Google site verification #408

Merged

		REFASTER_FOLDER="${WEBSITE_FOLDER}/refastertemplates"
		REFASTER_DOCS_FOLDER="${DOCS_FOLDER}/refastertemplates"

		@@ -0,0 +1,5 @@
		source "https://rubygems.org"
		gem "github-pages", "227"

Set up documentation website generation and deployment #253

Set up documentation website generation and deployment #253

Conversation

japborst commented Sep 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rickie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rickie commented Sep 27, 2022

Choose a reason for hiding this comment

japborst commented Sep 27, 2022

rickie left a comment • edited by Stephan202 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

japborst Sep 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rickie commented Sep 28, 2022

japborst commented Sep 28, 2022

rickie left a comment • edited Loading

Choose a reason for hiding this comment

japborst commented Oct 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

japborst commented Oct 1, 2022

Stephan202 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

japborst commented Oct 2, 2022 • edited Loading

japborst commented Oct 3, 2022

rickie left a comment

Choose a reason for hiding this comment

maxsumrall commented Oct 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stephan202 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

japborst commented Sep 21, 2022 •

edited

Loading

rickie left a comment •

edited by Stephan202

Loading

japborst Sep 28, 2022 •

edited

Loading

rickie left a comment •

edited

Loading

japborst commented Oct 2, 2022 •

edited

Loading