-
Notifications
You must be signed in to change notification settings - Fork 61
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
feat: ability to retrieve logs #478
Conversation
this just uses too much memory when done this way
by default, the primary container will be used, as before. this can be changed by env var JAM_TARGET_CONTAINER. e.g. `JAM_TARGET_CONTAINER=secondary npm start`
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.
Nicely done! Tested locally on regtest, and everything worked.
Not sure about the primary/secondary naming, as mentioned, but maybe I'm misunderstanding things.
src/setupProxy.js
Outdated
ws: true, | ||
}) | ||
) | ||
const SECONDARY_CONTAINER_JAM_API_PORT = 29080 |
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.
Not sure about the PRIMARY
& SECONDARY
naming. Shouldn't it be standalone
and ui-only
?
The primary/secondary distinction is for our regtest setup only, or is it not?
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.
You've hit a real pain point.
First off, I totally agree - ui-only
and standalone
is already way better naming.
primary
and secondary
is just too tightly coupled to the regtest env and the docs mention npm start
as an alternative to running the docker images.
However, ui-only
and standalone
are the terminologies to different the docker distributions.
In this case, "secondary" is really using a standalone
docker image as backend. But "primary" does not use ui-only
, it just "mimics" it with setupProxy
. This might lead to confusion (at least it is already confusing to me to some extent).
Just for the record:
Imho, the correct approach would be, instead of advocating using npm start
to access a mainnet jm instance,
building Jam (with npm build
) and delivering it via a real web server. But that seems overly complicated for "ordinary" users - that's why the ui-only
docker image is so handy - it does this out-of-the-box.
Having said that - it is nice that npm start
also works as a quick way to start Jam and we should aim to keep it that way.
Thanks for your input 🧡
I'll try to come up with a more suitable approach.
Introduced "backend types" Also, with the docker image merged and built, one can use |
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.
Introduced "backend types"
native
andjam-standalone
in thesetupProxy
script.
I like it! Way less confusing now.
tACK, Looks good to me ✅
Closes #469.
First iteration of "feat: ability to retrieve logs #469".
This PR uses features implemented in joinmarket-webui/jam-docker#51 and must be tested in conjunction with it.
Features:
colorize lines by log level(reverted: uses too much memory)Possible improvements for the next iteration:
Copying a phrase from the other PR:
Also, if you have any questions or are unsure about anything - don't hold back! Happy to answer and discuss any upcoming doubts/thoughts/ideas.
OLD How to test (outdated: see below)
serve-logs
fromjam-docker
. See PR feat: Serve logs jam-docker#51 for more details.npm run regtest:rebuild
, but:npm run regtest:clear && docker-compose \ --env-file docker/regtest/.env.example \ --file docker/regtest/docker-compose.yml build
(why? -> because
--pull
is not supported if you have a locally built image in your docker-compose setup)npm start
, run:How to test (up-to-date)
npm run regtest:rebuild
npm run start:regtest:secondary
📸
📸 reverted changes (just for reference; might re-implement in future iterations)