-
Notifications
You must be signed in to change notification settings - Fork 246
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
chore!: cleanup unused cmd #6045
base: develop
Are you sure you want to change the base?
Conversation
Looks like you have BREAKING CHANGES in your PR. Check-list
|
Jenkins Builds
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #6045 +/- ##
===========================================
- Coverage 60.88% 60.84% -0.05%
===========================================
Files 811 811
Lines 108894 108894
===========================================
- Hits 66303 66252 -51
- Misses 34841 34884 +43
- Partials 7750 7758 +8
Flags with carried forward coverage won't be shown. Click here to find out more. |
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.
I don't know if I like the removal of setup in _assets/compose
and _assets/systemd
for mailserver
. It is still quite a valid part of the setup. Mailservers(waku v1 nodes) are still being used in notify.prod
fleet, so I don't think removing this stuff makes sense.
A poorly named "Mailserver" is essentially a Whisper node that stores message history in either a LevelDB or PostgreSQL database. | ||
|
||
A Status app user can run their own Mailserver for faster message retrieval or additional security. |
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.
Why not just move this and to BOOTNODE.md
_docs
?
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.
Well, BOOTNODE.md
is about running cmd/bootnode
, doesn't make sense to keep it if we remove the app.
MAILSERVER.md
runs cmd/statusd
(which stays for now), so I suppose we could keep it indeed. Though it basically shouldn't be used at this point. If someone needs to run a Mailserver, it's better to checkout some older version, running anything WakuV1 from current state has no guarantees to be correct.
@jakubgs do we have plans to kick notify.prod
?
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.
As far as I know we can't unless we implement support for Gorush directly in nim-waku
, and that's not happening anytime soon.
I'm fine with removing bootnode
stuff.
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.
@jakubgs: Mailservers(waku v1 nodes) are still being used in notify.prod fleet,
Why is notify.prod using wakuv1? Is this fleet used only by those users that have not updated their clients to more recent versions?
Removed:
cmd/bootnode
cmd/spiff-workflow
cmd/node-canary
cmd/statusd-prune
Checking clients: