Skip to content
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

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from
Open

chore!: cleanup unused cmd #6045

wants to merge 5 commits into from

Conversation

igor-sirotin
Copy link
Collaborator

@igor-sirotin igor-sirotin commented Nov 4, 2024

Removed:

  • cmd/bootnode
  • cmd/spiff-workflow
  • cmd/node-canary
  • Some outdated/unused files
  • Cleanup references to some non-existent cmd/statusd-prune

Checking clients:

Copy link

github-actions bot commented Nov 4, 2024

Looks like you have BREAKING CHANGES in your PR.
Please make sure to follow 💔How to introduce breaking changes guide:

Check-list

@status-im-auto
Copy link
Member

status-im-auto commented Nov 4, 2024

Jenkins Builds

Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 3180042 #1 2024-11-04 13:50:15 ~4 min windows 📦zip
✔️ 3180042 #1 2024-11-04 13:50:28 ~4 min tests-rpc 📄log
✔️ 3180042 #1 2024-11-04 13:50:53 ~4 min macos 📦zip
✔️ 3180042 #1 2024-11-04 13:51:03 ~5 min ios 📦zip
✔️ 3180042 #1 2024-11-04 13:51:06 ~5 min macos 📦zip
✔️ 3180042 #1 2024-11-04 13:51:06 ~5 min linux 📦zip
✔️ 3180042 #1 2024-11-04 13:52:10 ~6 min android 📦aar
✔️ 3180042 #1 2024-11-04 14:19:52 ~33 min tests 📄log

Copy link

codecov bot commented Nov 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.84%. Comparing base (b329b15) to head (3180042).

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     
Flag Coverage Δ
functional 13.08% <ø> (+<0.01%) ⬆️
unit 60.20% <ø> (-0.06%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 39 files with indirect coverage changes

Copy link
Member

@jakubgs jakubgs left a 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.

Comment on lines -3 to -5
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.
Copy link
Member

@jakubgs jakubgs Nov 4, 2024

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 BOOTNODE.md to _docs?

Copy link
Collaborator Author

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?

Copy link
Member

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.

Copy link
Member

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants