vendor on npm prepare instead of committing to git#7423
vendor on npm prepare instead of committing to git#7423
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #7423 +/- ##
=======================================
Coverage 80.42% 80.42%
=======================================
Files 732 732
Lines 31055 31055
=======================================
Hits 24975 24975
Misses 6080 6080 Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Overall package sizeSelf size: 4.56 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------| | import-in-the-middle | 2.0.6 | 81.92 kB | 813.08 kB | | dc-polyfill | 0.1.10 | 26.73 kB | 26.73 kB |🤖 This report was automatically generated by heaviest-objects-in-the-universe |
|
✅ Tests 🎉 All green!❄️ No new flaky tests detected 🔗 Commit SHA: 94ee4de | Docs | Datadog PR Page | Was this helpful? Give us feedback! |
rochdev
force-pushed
the
vendor-on-prepare
branch
4 times, most recently
from
f2d0c36 to
cebc30d
Compare
There was a problem hiding this comment.
I see we have a problem in .github/workflows/update-3rdparty-licenses.yml that it only triggers on updates to the root yarn.lock. This means it didn't trigger on this PR. We need to make sure that it also triggers on changes to the new vendor/package-lock.json. Can you make a change to the action, so it runs here as well? And now that you're at it, could you make sure it also triggers on changes to .github/vendored-dependencies.csv?
| - reopened | ||
| - synchronize |
There was a problem hiding this comment.
This should be kept. We should trigger workflows again, if a PR was reopened or updated
There was a problem hiding this comment.
I guess my worry is what if dependabot opens a PR, then a malicious user adds a commit, and then dependabot updates the PR? In that scenario, the PR would be auto-approved by automation without ever validating the changes from the user.
There was a problem hiding this comment.
This might be an existing problem and out of scope of this PR though, so I can add it back for now.
rochdev
force-pushed
the
vendor-on-prepare
branch
from
cebc30d to
94ee4de
Compare
BridgeAR
left a comment
BridgeAR
left a comment
There was a problem hiding this comment.
The license script is now not picking up the vendored dependencies anymore. We should include those though.
cc @watson |
3 participants
Please make sure your changes are properly tested!
What does this PR do?
Vendor on npm prepare instead of committing to git.
Motivation
The choice to commit vendored dependencies was to get slightly better install times locally and in CI, and being able to install from git regardless of package manager. However, these are very small benefits, and the complexity of our CI automation has exploded because of the need to automatically re-vendor after the automation. It also has the downside that every time we touch the bundler config there are dozens of files changed every time. At this point I don't think the trade-offs are worth it to keep the files in git. Vendoring on prepare makes everything much simpler.
Additional Notes
Only the files outside of
vendor/distneed review, everything else is just the dist folder being deleted.