Add package-lock.json files to user tests #35341
Conversation
jablko
force-pushed
the
patch-29
branch
3 times, most recently
from
768134d to
94ba3e6
Compare
jablko
force-pushed
the
patch-29
branch
3 times, most recently
from
9d62246 to
7c6f55a
Compare
jablko
force-pushed
the
patch-29
branch
4 times, most recently
from
2e8052f to
d69c888
Compare
sandersn
left a comment
sandersn
left a comment
There was a problem hiding this comment.
This idea seems reasonable, but we need one of us to commit to watch runs after merging.
@weswigham does this look OK to you? Are you able to make sure this works correctly if we decide to merge it?
sandersn
left a comment
sandersn
left a comment
There was a problem hiding this comment.
After discussing with @weswigham in person, we decided
- Lack of package-lock hasn't been a problem for PR builds so far.
- I worry that package-locks will cause problems keeping builds up to date.
- Neither of us are sure that the mechanism to distinguish PR builds from regularly triggered builds is correct.
So I don't think we'll take this PR.
| } | ||
| if (fs.existsSync(path.join(cwd, "package.json"))) { | ||
| if (fs.existsSync(path.join(cwd, "package-lock.json"))) { | ||
| if (process.env.TRAVIS_EVENT_TYPE === "cron" && fs.existsSync(path.join(cwd, "package-lock.json"))) { |
There was a problem hiding this comment.
Just leaving the lockfile in place wouldn't be enough to make it get used, it'd also need to be committed (otherwise no PR build is going to have them anyway, as it's always a fresh clone for a CI build), and installation would need to use npm ci rather than npm i (since npm i updates the lockfile by default). Then, for this whole scheme to be usable, we'd need to ensure that the package-lock.json updates are included in and themselves create a "user test baselines updated" PR (meaning we'd need to "fail the build" and open a PR if any package locks update).
Oh, and all this is only actually useful if someone actually reviews those package-lock updates and understands them and the effects they have on the associated builds, rather than always just checkbox-merging them; otherwise we may as well just not have a lockfile, since we're essentially blindly installing the latest allowable packages anyway, and we may as well cut the busywork.
There was a problem hiding this comment.
Yes to everything you've said. Would it be possible to include package-lock.json files in the "User test baselines have changed" PRs, but continue to create a PR only if the baselines themselves have changed?
I.e. add them to the existing "User test baselines have changed" PRs, but don't create a PR/fail the build for package-lock.json-only changes?
jablko
force-pushed
the
patch-29
branch
8 times, most recently
from
4b9c727 to
d17dd5b
Compare
|
@typescript-bot user test this |
|
@typescript-bot user test this (sorry, typescript-bot only responds to team members) |
|
This PR hasn't seen any activity for quite a while, so I'm going to close it to keep the number of open PRs manageable. Feel free to open a fresh PR or continue the discussion here. |
|
This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise. |
4 participants
Try to use the same package versions that resulted in the baselines. Update the package versions in rolling builds and reuse them in triggered PR associated builds.