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.32% -0.10%
==========================================
Files 732 734 +2
Lines 31055 31602 +547
==========================================
+ Hits 24975 25384 +409
- Misses 6080 6218 +138
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.77 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------| | import-in-the-middle | 2.0.6 | 81.92 kB | 816.75 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: 83cf0ea | Docs | Datadog PR Page | Was this helpful? React with 👍/👎 or 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?
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 |
|
I think when I added support for multiple |
Since we do effectively the same work in JS for the validation, would it make sense to just generate it locally instead? Why do we need a separate project in Python to handle this? |
Hmm good question. The pro for using the Python project, is that all Datadog repos use it, so it's guaranteed to follow our official requirements, any improvement or change in behavior in it benefit us all. You could reverse the question and ask if we should keep maintaining our own js-variant. I've already implemented support for Additionally, our own JS-variant will need significant updates to do the same work that the Python tool currently does. As far as I know, our JS-variant only validates that a dependency exists - it doesn't fetch license info from GitHub and npm. |
|
I actually agree that having a unified tool is best. That way everyone always benefits most. We should just remove our JS variation in that case. |
|
@watson Updated with your branch and it seems to work now. Thanks for the assist unblocking this PR! |
Co-authored-by: Thomas Watson <[email protected]>
BridgeAR
left a comment
BridgeAR
left a comment
There was a problem hiding this comment.
Almost LGTM. I left a few questions how to handle silent and reproducing test cases as users would face it.
| - uses: ./.github/actions/node/active-lts | ||
| - run: FILENAME=$(npm pack --pack-destination /tmp) && mv /tmp/$FILENAME /tmp/dd-trace.tgz | ||
| - uses: ./.github/actions/install | ||
| - run: FILENAME=$(npm pack --silent --pack-destination /tmp) && mv /tmp/$FILENAME /tmp/dd-trace.tgz |
There was a problem hiding this comment.
Should we not keep the output to know what happened? The same about other jobs.
There was a problem hiding this comment.
The output is used as input to the next task. Also, this was already not the case before.
Co-authored-by: Ruben Bridgewater <[email protected]>
|
@watson Any further concerns now that 3rd party dependencies are fixed? |
BridgeAR
dismissed
watson’s stale review
Dismissing to unblock releasing (the update script is currently blocked and this will likely resolve the problem). All blocking aspects should be resolved.
* remove vendor dist folder * switch to npm for vendoring * update license attribution script
)" This reverts commit 2608ddb. System tests failed: DataDog/system-tests/actions/runs/22501109173/job/65191588666?pr=6394
* remove vendor dist folder * switch to npm for vendoring * update license attribution script
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.