add legal-docs as a dependency w/ symlinks; fixes #738

[clouserw]

@pdehaan mentioned that the legal-docs repository has a package.json file in it so we can include it as a dependency. This would make the patch super easy and is what I've done below.

r? @fzzzy for the overall patch. One thing I noticed is that the old file was named de.md and the one in the legal-docs repo is de-DE.md. I remember the bugs where de-DE content wouldn't show up for de, and we might be hitting that again here. I'll check with legal.

r? @eleemoz In case you're reading github-mail: The German Test Pilot legal docs (terms and privacy policies) are specifying they are German for Germany (de-DE) instead of just German (de). Is that on purpose? If not, I'd like to change it.

r? @relud because I want to make sure symlinks will work like this when we deploy. Any concerns with this? Also, does our environment get recreated with each deploy so it would re-download these fresh or do we have to think about stale content here?

[codecov-io]

Codecov Report

Merging #2357 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff           @@
##           master   #2357   +/-   ##
======================================
  Coverage    95.5%   95.5%           
======================================
  Files         106     106           
  Lines        2445    2445           
======================================
  Hits         2335    2335           
  Misses        110     110

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a7e1a4a...17e892e. Read the comment docs.

[fzzzy]

The technical details here look correct to me.

As for the deployment question, I believe the build process will see the symlinked md files and thus kick off the correct builds, but then the output is just html files in the build directory, so nothing should change in terms of deployment.

[relud]

I want to make sure symlinks will work like this when we deploy. Any concerns with this?

I just tested, and symlinks work with awscli. I can't say for certain because these files are copied by the build process, but stage will tell all.

Also, does our environment get recreated with each deploy so it would re-download these fresh or do we have to think about stale content here?

our deployment does not git clean between deploys, and also it uses the following Dockerfile to build, which may cache after npm install when package.json doesn't change if I understand docker correctly:

FROM node:6.2.0
WORKDIR /app
COPY package.json /app/package.json
RUN npm install
COPY addon/package.json /app/addon/package.json
RUN cd addon && npm install
COPY . /app
CMD cd addon && npm run sign && cd .. && NODE_ENV=production ENABLE_PONTOON=0 npm run static

I could reconfigure deploys to build everything from scratch if that is desired though.

[relud]

this appears to cause the follow problem during npm run static in builds:

Error: Cannot find module 'tos-pp' from '/home/jenkins/slave/workspace/pipelines/testpilot/addons/reludtest'
    at /home/jenkins/slave/workspace/pipelines/testpilot/addons/reludtest/node_modules/browser-resolve/node_modules/resolve/lib/async.js:46:17
    at process (/home/jenkins/slave/workspace/pipelines/testpilot/addons/reludtest/node_modules/browser-resolve/node_modules/resolve/lib/async.js:173:43)
    at ondir (/home/jenkins/slave/workspace/pipelines/testpilot/addons/reludtest/node_modules/browser-resolve/node_modules/resolve/lib/async.js:188:17)
    at load (/home/jenkins/slave/workspace/pipelines/testpilot/addons/reludtest/node_modules/browser-resolve/node_modules/resolve/lib/async.js:69:43)
    at onex (/home/jenkins/slave/workspace/pipelines/testpilot/addons/reludtest/node_modules/browser-resolve/node_modules/resolve/lib/async.js:92:31)
    at /home/jenkins/slave/workspace/pipelines/testpilot/addons/reludtest/node_modules/browser-resolve/node_modules/resolve/lib/async.js:22:47
    at FSReqWrap.oncomplete (fs.js:117:15)

[relud]

update: I just filed mozilla-services/cloudops-deployment#561 which changes the process used to build the static website and addons to be more idiomatic to jenkins. It removes the Dockerfile I mentioned two comments ago, and adds a git clean -fxd to deploys. This change would prevent issues with stale legal docs.

[lmorchard]

FWIW, I think we've tried to avoid symlinks in the repo (e.g. for de-DE locale), because they break for developers on Windows. But, I haven't tried this myself, so I can't say for sure.

[clouserw]

Thanks for the updates, @relud.

Regarding that npm error, the legal-docs repository doesn't have an index.json so it can't be require()'d, but we're not doing that, so it should be fine. It looks like the browser-resolve package (part of browserify?) is iterating all the packages and apparently trying to load them? That's a guess and this is a bit out of my depth. @fzzzy (or anyone) is this something you've seen before and/or easy to fix?

(side note) It looks like we're only using browserify in one place - do we need it?

[lmorchard]

(side note) It looks like we're only using browserify in one place - do we need it?

We're using Browserify in a few places - mainly here to build the app.js & vendor.js bundles that run the web frontend.

[fzzzy]

@clouserw Given that Windows may not be happy with symlinks, and that the legal docs repo doesn't have a package.json which is causing problems, how about just adding an npm script that does a curl or wget and then moves things into the right place? Then we can just run it manually whenever updates are desired.

[fzzzy]

Alternatively, we could add a package.json to the legal repo (with just generic metadata fields) and change the gulp task that looks for the md files in compiled-templates to look for them in node_modules. That should work without problems on windows.

[clouserw]

@fzzzy and I chatted on IRC. New patch fixes the crash. I think this is good to go for engineering. Brian Smith is following up on the de vs de-DE question.

[learnedhoof]

@clouserw that was done by the localization team -- i don't know that there's a specific reason.

[learnedhoof]

@clouserw submitted PRs for changing from de-DE to de
mozilla/legal-docs#771
and mozilla/legal-docs#772

[clouserw]

Thanks @eleemoz . Those are closed now, so landing this.

[clouserw]

@SoftVision-PaulOiegas @SoftVision-CosminMuntean -- pinging you on this too. If this goes wrong, the legal docs won't show up on dev (potentially, just the german version also)