add yarn.lock file for deterministic builds#7041
add yarn.lock file for deterministic builds#7041erwinmombay merged 6 commits intoampproject:masterfrom
Conversation
| "gulp-git": "1.7.2", | ||
| "gulp-help": "1.6.1", | ||
| "gulp-if": "2.0.0", | ||
| "gulp-image-diff": "0.3.0", |
There was a problem hiding this comment.
had to remove this since it had compatibility issues. i dont think we use it right now anyways
|
Thanks. I assume this would have prevented #7039 cc / @ipostelnik |
|
@erwinmombay I assume Maybe we can start by a public |
|
@aghassemi yeah we will have to update docs for sure. I'll create an issue |
|
@erwinmombay I am more worried about the work involved in migrating travis config, few scripts in package.json and our internal release scripts to yarn than just the docs. Would be nice to do all that in a single PR after which everyone should switch to yarn. |
|
the work on travis is pretty minimal the only tricky part is installing "yarn" |
93cfd99 to
b3ceb33
Compare
| - NPM_CONFIG_PROGRESS="false" | ||
|
|
||
| cache: | ||
| yarn: true |
0fb8c04 to
732e98e
Compare
d33fb6c to
1a23f67
Compare
|
Here is a list of owners that can approve this PR: /to @cramforce Owners can approve through the "APPROVED" label. If the author of the PR is an owner of the files or directory, the PR will be approved right away. |
|
@ampprojectbot You will need a more optimistic profile image for me to approve this change! |
build-system/pr-check.js
Outdated
| return 0; | ||
| } | ||
|
|
||
| if (!(files.some(x => x === 'package.json') && |
There was a problem hiding this comment.
The logic here fails if PR does not include either of them. We want logical XOR here. Maybe do:
if( files.includes('package.json') ? files.includes('yarn.lock') : !files.includes('yarn.lock') )
There was a problem hiding this comment.
good catch. will fix
There was a problem hiding this comment.
can you check my logic again. changed it to:
files.includes('package.json') ? !files.includes('yarn.lock') : false
we should be OK if yarn.lock is updated but package.json is not in instances where we re-pin nested deps. unlikely but i see it happen when running yarn update dep
build-system/pr-check.js
Outdated
|
|
||
| if (!(files.some(x => x === 'package.json') && | ||
| files.some(x => x === 'yarn.lock'))) { | ||
| console.error(`\npr-check.js - please update through yarn.\n`); |
There was a problem hiding this comment.
pr-check.js - any update to package.json or yarn.lock must include the other file. Please update through yarn.
build-system/tasks/index.js
Outdated
| require('./get-zindex'); | ||
| require('./lint'); | ||
| require('./make-golden'); | ||
| //require('./make-golden'); |
There was a problem hiding this comment.
remove instead of commenting out?
|
Here is a list of owners that can approve this PR: /to @cramforce Owners can approve through the "APPROVED" label. If the author of the PR is an owner of the files or directory, the PR will be approved right away. |
|
bot logic was wrong. fixing here https://github.com/google/github-owners-bot/pull/7/files |
1a23f67 to
c1cf5e7
Compare
c1cf5e7 to
f276cf1
Compare
build-system/pr-check.js
Outdated
| return 0; | ||
| } | ||
|
|
||
| if (files.includes('package.json') ? !files.includes('yarn.lock') : false) { |
There was a problem hiding this comment.
I think what I suggested previously was correct:
if has package.json => must also have yarn.lock, otherwise must NOT have yarn.lock
* add yarn.lock file for deterministic builds * switch travis to use yarn * fix logic for package.json an yarn.lock check * make sure that it also errors if only yarn.lock exists * add back yarn.lock file * update serving/building docs
* add yarn.lock file for deterministic builds * switch travis to use yarn * fix logic for package.json an yarn.lock check * make sure that it also errors if only yarn.lock exists * add back yarn.lock file * update serving/building docs
/to @dknecht as well for review
need to use
yarninstead ofnpm installin deployment systems