-
Notifications
You must be signed in to change notification settings - Fork 12.9k
Add package-lock.json files to user tests #35341
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
Conversation
768134d
to
94ba3e6
Compare
9d62246
to
7c6f55a
Compare
2e8052f
to
d69c888
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -67,7 +67,7 @@ namespace Harness { | |||
cwd = config.path ? path.join(cwd, config.path) : submoduleDir; | |||
} | |||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
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. |
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.