Page MenuHomePhabricator

[ecash-agora] Patch CI for prod install of ecash-wallet
ClosedPublic

Authored by bytesofman on Thu, Oct 30, 21:11.

Details

Reviewers
Fabien
Group Reviewers
Restricted Project
Commits
rABC6e7ad2438887: [ecash-agora] Patch CI for prod install of ecash-wallet
Summary

Now that it is not a dev dep, needs to be installed as a real dep

I tried to simply omit the "-D" in the ecash-wallet install, but the strange error about ecash-lib being unavailable persisted (could recreate what I see in CI locally).

With this change, it builds locally. This is (probably) a better way to do it, removing all local deps and evidence of them and then regenerating a package-lock.json with an npm install

I am not sure exactly what is causing the npm install ecash-lib@latest failure. I can do that locally. But the docker container doesn't like it.

Test Plan

docker build -f ecash-agora.Dockerfile -t ecash-agora_local .

Re-run CI to publish a new version

Diff Detail

Repository
rABC Bitcoin ABC
Branch
ecash-agora-dockerfile-patch
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 34916
Build 69294: Build Diff
Build 69293: arc lint + arc unit

Event Timeline

update to change that passes test plan

bytesofman edited the test plan for this revision. (Show Details)
bytesofman edited the summary of this revision. (Show Details)
Fabien requested changes to this revision.Thu, Oct 30, 21:50
Fabien added a subscriber: Fabien.
Fabien added inline comments.
ecash-agora.Dockerfile
22 ↗(On Diff #56370)

We should stick to npm ci otherwise you don't know what environment you're building/testing against

This revision now requires changes to proceed.Thu, Oct 30, 21:50
bytesofman marked an inline comment as done.
bytesofman added inline comments.
ecash-agora.Dockerfile
22 ↗(On Diff #56370)

this would be ideal. Unfortunately we are already making a trade-off here to use the npmjs deps to make everything work better for non-monorepo users --- we already don't know this.

the previous script is overwriting package-lock.json every time it runs npm install to pull in the modules from npm

So we are moving from doing this 3 times to doing it one time.

bytesofman added inline comments.
ecash-agora.Dockerfile
13 ↗(On Diff #56370)

Pretty sure the issue is this

  • docker file installs ecash-lib from online
  • but at this point, it still has ecash-wallet as a local dep, and the local-dep ecash-wallet has ecash-lib as a local dep
  • so ecash-lib conflicts and we get an error

to keep all the deps for the published version the published deps, we need to do first replace all the local deps with published deps, then npm install

ecash-agora.Dockerfile
22 ↗(On Diff #56370)

I don't get it, you can use the npmjs dependencies but still use npm ci to stick to pinned versions ? Otherwise you can't know what your dependency chain is. Our own dependencies are fine, we manage them but third party ones need to be pinned

bytesofman added inline comments.
ecash-agora.Dockerfile
22 ↗(On Diff #56370)

we have to update the package-lock.json in this dockerfile somewhere in order to use the published deps; this was already happening in the old version as running npm install ecash-lib also updates the whole package-lock.json; then we were just calling npm ci after that on the updated package-lock.json

The pinned versions in package.json would ensure those deps remained the same, but their transitive deps could have been changing before this diff (tho there are none in this lib, so moot point)

so, we could also call npm ci again after our npm install here ... but there is not really a benefit to doing this, we've already installed what generated the package-lock.json in this docker container.

More to the point for this diff though -- ecash-agora has no dependencies except for ecash-lib, chronik-client, and ecash-wallet

The other stuff in the package.json is just dev dependencies which are not included in the published package; this was the original problem that triggered the need for this change: ecash-wallet was a dev dep so it wasn't getting into the published package (which now actually uses it in published methods), causing errors in apps installing the latest versions.

This revision is now accepted and ready to land.Fri, Oct 31, 12:40
This revision was automatically updated to reflect the committed changes.
bytesofman marked an inline comment as done.