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

no need for the BUILD_FILES env var #122

Merged
merged 9 commits into from
Nov 20, 2020

Conversation

peterbe
Copy link
Contributor

@peterbe peterbe commented Nov 20, 2020

Here's the problem, it seems the yarn install stuff isn't getting cached at all.
Screen Shot 2020-11-20 at 10 19 17 AM

Cool! It totally works.
Screen Shot 2020-11-20 at 10 47 26 AM

Now, taking care of yarn and node_modules takes ~7s on a warm cache. And a warm cache is extremely likely on this repo because 9 out of 10 PRs are expected to be edits to the files/**/index.html files.

Unlike the "default suggestion for Node Yarn" I opted for the same solution we're doing for the poetry install in Yari.
And this is actually the recommended way for "Node - Lerna".
The net effect is that we cache the whole node_modules directory. And it's keyed by OS and by yarn.lock so it's very unlikely that a cache hit is stale.

@peterbe peterbe requested review from a team and fiji-flo November 20, 2020 13:22
@peterbe peterbe requested a review from a team as a code owner November 20, 2020 13:45
@peterbe peterbe requested review from chrisdavidmills and removed request for chrisdavidmills and fiji-flo November 20, 2020 13:45
# The reason this script isn't in `package.json` is because
# you don't need that script as a writer. It's only used in CI
# and it can't use the default CONTENT_ROOT that gets set in
# package.json.
node node_modules/@mdn/yari/build/cli.js
node node_modules/@mdn/yari/build/cli.js ${{ env.GIT_DIFF }}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change was the original thing I wanted to change. Now the build/cli.js is a caporal CLI app, its default argument is (0-N) the file paths to the index.html files to build. This used to not be possible before.
Depending on BUILD_FILES never felt great because it's a string that you need to manually split up. By relying on UNIX principles (aka. bash and caporal) you're instead letting them take care of that.

@peterbe
Copy link
Contributor Author

peterbe commented Nov 20, 2020

@escattone This PR became a double-concern thing. I first wanted to change how we invoke the builder based on the git diff (from get-diff-action). Then I noticed that the Yarn caching always failed!
First, when I tried to fix the yarn caching problem I tried a couple of different things and debugging. But the best solution I could ever come up with was to basically cache the HTTP traffic that yarn install would eventually have to depend on. Well, what if we can not do the yarn install at all, if we don't really have to. It's the same thing I did in https://github.com/mdn/yari/pull/1767/files
It used to be that the best-case was that yarn install would not need to do any HTTP downloads from npmjs.com but would still need to the disk I/O of unpacking moving tarballs from its internal cache and exploring them into $CWD/node_modules/. So why not bypass the whole enchilada?

Copy link
Contributor

@escattone escattone left a comment

Choose a reason for hiding this comment

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

@peterbe I like it.

@escattone escattone merged commit 213e235 into mdn:main Nov 20, 2020
@escattone
Copy link
Contributor

@peterbe I don't know why the core-yari-content team was added as a reviewer. The one file this PR changes isn't "code-owned" by that team. Strange.

@peterbe peterbe deleted the no-need-for-the-build_files-env-var branch November 20, 2020 18:42
@peterbe
Copy link
Contributor Author

peterbe commented Nov 20, 2020

@peterbe I don't know why the core-yari-content team was added as a reviewer. The one file this PR changes isn't "code-owned" by that team. Strange.

Probably because, at least at some point, I did make an edit to one of the files/en-us/**/index.html files so the get-diff-action would trigger at all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants