Conversation
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
kriszyp
left a comment
There was a problem hiding this comment.
I don't think this worked (I don't think it installed any dependencies).
|
Tests are failing due to HarperFast/harper#325 |
|
@kriszyp when you have a few spare cycles, I'd love your input on this change! See the PR description for the bullet points of what I propose changing and why. |
The dependencies in the package.json are maintained by our core repo, this will reduce noise. Instead, we’ll make a habit of regularly synchronizing core into here.
|
HarperFast/harper#327 will fix the integration test failures, I believe. |
I just watched a documentary on the Titan submersible tragedy, so this commit title feels… wrong… in all the right ways. ;D
cb1kenobi
left a comment
There was a problem hiding this comment.
LGTM.
So what's the general flow for updating our local repo? git pull origin main && npm run core:sync?
|
It depends on if you want to pull in changes in core or not, the sync script will change the commit it references for the sub module if a newer one is available on main. Make sense? |
|
@cb1kenobi so probably |
|
Yay, socket is happy now! 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 |
|
... we'll wait to merge this PR until Kris gets back and has a chance to look at it, IMO! |
|
I have been doing |
|
I mean, we could add husky to the mix, that's the typical solution to that. But then it'll run whenever you switch branches which can get really annoying if you're a rapid branch switcher during rebases, etc. |
kriszyp
left a comment
There was a problem hiding this comment.
So does core-sync update harper-pro submodule reference to point to the latest in the current core branch? I thought git submodule update did the reverse, it updated the core branch/commit to the reference currently pointed to by the parent.
And what if I don't want to run npm install when I update the reference to core (or update core to the reference), because I don't want it to remove my npm symlinks? (which is the case 99% of the time for me)
|
What npm links do you have? The submodule gets pointed to the latest from the main branch of harper - I think I have an extra update in there, confusing things. The remote update is the one that actually does something. I pulled in the fix for the exported port env var with this script, for example. And then the removal of the old node 16 based free space module. So I know it's functional. I'm envisioning this being run from a workflow that gets invoked whenever core is updated, opening a PR here with the changes applied. That will keep the dependencies in sync, and let the tests in pro run. |
rocksdb-js, lmdb-js, msgpackr, cbor-extract, ordered-binary, weak-lru-cache.
What if I want to point to a different branch? This is something I frequently do for coordinated PRs (and it is very intuitive because it is just using git). |
|
I mean, you can still do it manually :) with the workflow we could pass the branch name as an arg |
|
@kriszyp I added a script to set the tracked branch by name, which is a passthrough to the appropriate git command. I also added a flag to skip NPM install, which will place the lockfile into an inconsistent state, but it will maintain your links. An untested workflow was created for synchronizing core, too. The permissions of it might be off. |
Co-authored-by: Chris Barber <chris@harperdb.io>
kriszyp
left a comment
There was a problem hiding this comment.
Ok, that sounds good, and yeah, it seems fine and easy to just use standard git commands when I am updating core without any dependency changes.
Also: my intention was that @datadog/pprof be a dependency of harper-pro and not harper (which shouldn't be using). Maybe it never got removed from harper (and should be), but how do we maintain dependency "additions" of harper-pro?
|
@kriszyp for that, we'll need to do some more engineering, or maintain the dependencies manually. Or actually rely on |
|
Created #74 to track that work |
I introduced a new
npm run core:syncscript to make core sync easier, and to take over some of whatnpm run packagewas doing with dependencies.With that new core sync, we now:
npm installafter that, which will update the lockfile with our pro particulars, but leave the dependency versions untouched.That means the
packageprocess is stable, with regards to versions:npm ciinstead ofnpm install.@datadog/pprofwas already a dependency of core, we don't need tonpm install @datadog/pprofas a result -- which was updating the version at package time to an untested, unvalidated version.The end result of all this should be: