test: build loopback.io to validate docs package #1254

virkt25 · 2018-04-12T21:49:10Z

Simpler approach to #1251

A simple script clone's loopback.io, bootstrap's it and attempts to build the site. If it fails, CI will fail.

Not adding it as a regular test since the script is not Windows compatible.

~~The 2 items from #1251 with regards to changes to loopback.io repo would still need to happen.~~ (DONE)

fixes #1232

Checklist

npm test passes on your machine
New tests added or existing tests modified to cover all changes
Code conforms with the style guide
API Documentation in code was updated
Documentation in /docs/site was updated
Affected artifact templates in packages/cli were updated
Affected example projects in examples/* were updated

raymondfeng · 2018-04-12T21:57:05Z

bin/docs.test.sh

+
+rm -rf loopback.io
+git clone https://github.com/strongloop/loopback.io.git
+npm run bootstrap


You should be able to clone loopback.io into sandbox instead of creating its own location.

The other option is to have loopback.io repo checked out somewhere else and try the following:

cd loopback.io npm i ~/Projects/loopback-next/docs --no-save

Good call for using sandbox.

We would still need to run the other commands (fetch-readmes, bundle install, jekyll build) since it's the build step that verifies the site hasn't broken due to formatting errors in docs, missing files, etc.

Make it a npm script of loopback.io package, such as npm run build.

Something like this? loopbackio/loopback.io#666

shimks · 2018-04-13T14:11:49Z

Can we turn the shell script into a js script so that it can work on windows (or is that not the problem with Windows compatibility)?

virkt25 · 2018-04-13T14:27:22Z

The problem is the requirement of having ruby installed on the machine + the fact that jekyll doesn't officially support Windows (See https://jekyllrb.com/docs/windows/) ... as such I don't think a script is worth the effort because if it fails, people are disappointed but if it doesn't exist there is no expectation of it working on Windows.

raymondfeng · 2018-04-13T15:30:38Z

.travis.yml


 jobs:
  include:
+    - stage: loopback.io


When does this job run? For all builds?

Yea, it would run for all PRs. I'm not sure how to run it if there are changes to just the @loopback/docs folder.

This is pretty worrisome then.

In the shell script, we should use git diff --name-only to find out if any files are changed under docs. If not, skip it.

Any reason this is worrisome? Besides Travis taking longer?

I did find some inspiration for skipping a stage early here facebook/react#2000 I can try something similar.

Taking longer is worrisome.

git diff --name-only origin/master docs/ will tell if the local repo has diff from the origin/master.

Here is what I can see to optimize:

git describe --abbrev=0 gives us the latest release tag

git diff --name-only <tag> -- docs/ show docs/* changed since last release

b-admike · 2018-04-13T15:43:10Z

bin/docs.test.sh

@@ -0,0 +1,9 @@
+#!/bin/bash
+
+rm -rf sandbox/loopback.io/


Should we consider adding checks for status codes returned by those commands or would Travis handle it if any of them fail (I guess they are interdependent so maybe this is not needed)?

If any command exits with anything but a 0, Travis will fail the build stage.

I think @b-admike's point was that the shell script only reports the last command's exit code to travis.

raymondfeng · 2018-04-13T15:53:08Z

My improved version of the shell script:

#!/bin/bash
# Make sure we use the correct repo root dir
DIR=`dirname $0`
REPO_ROOT=$DIR/..
pushd $REPO_ROOT >/dev/null
rm -rf sandbox/loopback.io/
# Shadow clone is faster
git clone --depth 1 https://github.com/strongloop/loopback.io.git sandbox/loopback.io
npm run bootstrap
pushd $REPO_ROOT/sandbox/loopback.io/ >/dev/null
# The following 3 lines will be replaced with npm run build
bundle install
npm run fetch-readmes
bundle exec jekyll build
popd >/dev/null
popd >/dev/null

virkt25 · 2018-04-16T13:11:01Z

bin/docs.test.sh

+git clone --depth 1 https://github.com/strongloop/loopback.io.git sandbox/loopback.io
+npm run bootstrap
+pushd $REPO_ROOT/sandbox/loopback.io/ >/dev/null
+# The following 3 lines will be replaced with npm run build


@raymondfeng I don't think bundle install should be a part of the build script in loopback.io since the script can be used there ... and on local, it's a one time install process. If you agree, I already have the changes ready to land for loopback.io after which I can update this script.

bajtos · 2018-04-16T13:08:12Z

.travis.yml

+    - stage: loopback.io
+      before_install:
+        - |
+          if git diff --name-only --quiet origin/master docs/; then


Can we use $TRAVIS_BRANCH instead of origin/master? I am worried that a hard-coded branch name origin/master will not work in the future, once we create branches like v4.x.

See https://docs.travis-ci.com/user/environment-variables/#Default-Environment-Variables

~~I think it might be even better to use $TRAVIS_COMMIT_RANGE. I'll make the change.~~

nvm, it doesn't do what I thought it did, I'll use $TRAVIS_BRANCH

bajtos · 2018-04-16T13:10:58Z

bin/docs.test.sh

+rm -rf sandbox/loopback.io/
+# Shadow clone is faster
+git clone --depth 1 https://github.com/strongloop/loopback.io.git sandbox/loopback.io
+npm run bootstrap


IIUC, all other packages have been already bootstrapped and all we need is to bootstrap sandbox/loopback.io. To speed things up, can we tell lerna to bootstrap this single package only? Something along the following lines:

lerna bootstrap --scope loopback.io-workflow-scripts

Note that loopback.io-workflow-scripts is the package name as defined in loopback.io's package.json file:

https://github.com/strongloop/loopback.io/blob/cb67ad4c437503d7726845c09851b3256eceed16/package.json#L2

npm run bootstrap is normally called by the before_script hook which is skipped for the loopback.io stage so we can run bootstrap only once after we've had an opportunity to clone the loopback.io repo into the sandbox.

I see, I did not realise before_script is being skipped. However, do we still need to bootstrap all packages to test the website? Isn't it enough to bootstrap loopback.io and perhaps docs?

Good point! I wonder if bootstrapping only the loopback.io package is enough to make it bootstrap any packages that are dependencies (aka docs).

bajtos · 2018-04-16T13:11:31Z

bin/docs.test.sh

+git clone --depth 1 https://github.com/strongloop/loopback.io.git sandbox/loopback.io
+npm run bootstrap
+pushd $REPO_ROOT/sandbox/loopback.io/ >/dev/null
+# The following 3 lines will be replaced with npm run build


Should we reword this comment as a TODO? Should this PR wait until npm run build is implemented in loopback.io?

Already implemented. See loopbackio/loopback.io#666

Will merge it today, just wanted to make sure @raymondfeng was ok with excluding the bundle install from the build script.

b-admike

raymondfeng · 2018-04-19T21:22:29Z

bin/docs.test.sh

+rm -rf sandbox/loopback.io/
+# Shadow clone is faster
+git clone --depth 1 https://github.com/strongloop/loopback.io.git sandbox/loopback.io
+./node_modules/lerna/bin/lerna.js bootstrap --scope loopback.io-workflow-scripts


No need to use the full path. lerna bootstrap is good enough as npm sets up the path.

raymondfeng · 2018-04-19T21:23:15Z

bin/docs.test.sh

@@ -0,0 +1,17 @@
+#!/bin/bash
+
+set -ev


Is it intentional?

Yes. These are the recommended flags by Travis (https://docs.travis-ci.com/user/customizing-the-build#Implementing-Complex-Build-Steps) to make sure the build fails if any command in the script fails.

raymondfeng · 2018-04-19T21:24:32Z

bin/docs.test.sh

@@ -0,0 +1,17 @@
+#!/bin/bash


Rename it to verify-docs.sh?

raymondfeng · 2018-04-19T21:34:10Z

bin/docs.test.sh

+bundle install
+npm run build
+popd >/dev/null
+popd >/dev/null


Maybe we should clean up sandbox/loopback.io after the verification is done. If we run the script locally, the loopback.io package will be left over on disk.

Hmm ... my reason to leave it was in case we wanted to locally start the server to view the site (verify formatting / etc. beyond the test which makes sure Jekyll doesn't break).

May we can add scripts to clean the sandbox? or maybe we can have a local script for build:site ... and verify:docs would do a cleanup by default?

+1. Or the same script can have an arg to control if we are building or verifying.

raymondfeng · 2018-04-19T21:40:13Z

bin/docs.test.sh

+git clone --depth 1 https://github.com/strongloop/loopback.io.git sandbox/loopback.io
+./node_modules/lerna/bin/lerna.js bootstrap --scope loopback.io-workflow-scripts
+pushd $REPO_ROOT/sandbox/loopback.io/ >/dev/null
+bundle install


Should this be removed?

No, npm run build doesn't install Ruby Gems ... this allows the build script in loopback.io to be used for build without triggering install since the gems only need to be installed once.

As bundle install is an one time thing, why do we want to run it every time?

raymondfeng · 2018-04-19T21:41:06Z

bin/docs.test.sh

+./node_modules/lerna/bin/lerna.js bootstrap --scope loopback.io-workflow-scripts
+pushd $REPO_ROOT/sandbox/loopback.io/ >/dev/null
+bundle install
+npm run build


Should we record the exit code of the command above and exit the script with it? Otherwise, can travis still detect the failure?

See my comment above with regards to the set -ev. If my understanding about the flag is incorrect please let me know. I did make commits to this PR to verify the following and they all worked as expected:

loopback.io stage exits early with success if no changes to @loopback/docs

loopback.io stage builds successfully with changes to @loopback/docs

loopback.io stage fails if a change to @loopback/docs will break Jekyll

raymondfeng · 2018-04-20T19:30:14Z

package.json

    "pretest": "npm run clean && npm run build",
    "test": "node packages/build/bin/run-nyc npm run mocha --scripts-prepend-node-path",
+    "verify:docs": "npm run build:site && node packages/build/bin/run-clean \"sandbox/loopback.io\"",
+    "build:site": "/bin/bash ./bin/verify-docs.sh",


No need to refer /bin/bash as the shebang in the script already uses bash.

Co-Authored-By: Raymond Feng <raymondfeng@users.noreply.github.com>

virkt25 requested review from bajtos and raymondfeng as code owners April 12, 2018 21:49

virkt25 added needs-priority and removed needs-priority labels Apr 12, 2018

virkt25 mentioned this pull request Apr 12, 2018

test: build loopback.io submodule to ensure it works with docs #1251

Closed

9 tasks

virkt25 requested a review from a team April 12, 2018 21:50

raymondfeng reviewed Apr 12, 2018

View reviewed changes

shimks approved these changes Apr 13, 2018

View reviewed changes

raymondfeng reviewed Apr 13, 2018

View reviewed changes

b-admike reviewed Apr 13, 2018

View reviewed changes

virkt25 force-pushed the docs-test-1 branch 2 times, most recently from 18f138e to 84d81ea Compare April 16, 2018 03:31

virkt25 commented Apr 16, 2018

View reviewed changes

bajtos reviewed Apr 16, 2018

View reviewed changes

virkt25 force-pushed the docs-test-1 branch 2 times, most recently from caeafda to 2d429a3 Compare April 19, 2018 17:01

b-admike approved these changes Apr 19, 2018

View reviewed changes

raymondfeng reviewed Apr 19, 2018

View reviewed changes

bin/docs.test.sh

@@ -0,0 +1,17 @@

#!/bin/bash

Copy link

Contributor

raymondfeng Apr 19, 2018

Choose a reason for hiding this comment

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

Rename it to verify-docs.sh?

virkt25 reacted with thumbs up emoji

raymondfeng reviewed Apr 19, 2018

View reviewed changes

This was referenced Apr 20, 2018

[Docs] Local Site Scaffolding #1232

Closed

[CLI] Local docs command #1233

Closed

virkt25 force-pushed the docs-test-1 branch from 4300510 to 33a6864 Compare April 20, 2018 19:15

raymondfeng reviewed Apr 20, 2018

View reviewed changes

raymondfeng approved these changes Apr 20, 2018

View reviewed changes

test: build loopback.io to validate docs package

68f55f0

Co-Authored-By: Raymond Feng <raymondfeng@users.noreply.github.com>

virkt25 force-pushed the docs-test-1 branch from 0d14340 to 68f55f0 Compare April 20, 2018 22:15

virkt25 merged commit 3a3c6fa into master Apr 20, 2018

virkt25 deleted the docs-test-1 branch April 20, 2018 22:25

virkt25 removed the review label Apr 20, 2018

test: build loopback.io to validate docs package #1254

test: build loopback.io to validate docs package #1254

Uh oh!

Conversation

virkt25 commented Apr 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raymondfeng Apr 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shimks commented Apr 13, 2018

Uh oh!

virkt25 commented Apr 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raymondfeng commented Apr 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

virkt25 Apr 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

b-admike left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

virkt25 commented Apr 12, 2018 •

edited

Loading

raymondfeng Apr 12, 2018 •

edited

Loading

raymondfeng commented Apr 13, 2018 •

edited

Loading

virkt25 Apr 16, 2018 •

edited

Loading

raymondfeng Apr 19, 2018 •

edited

Loading