-
Notifications
You must be signed in to change notification settings - Fork 248
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
refactor: only use shards #5474
Conversation
Jenkins BuildsClick to see older builds (126)
|
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.
🙌
cbbdb93
to
3a2a08f
Compare
For the scenarios that are failing, I think we're sending the messages on the wrong pubsub topic, i.e.
I think this should have probably been sent on I'm investigating the other cases. |
@jrainville, @mprakhov The question is: |
@richard-ramos both of these can only be called when the user has already joined the community. |
I'll revert last commit then |
958551c
to
f4ad281
Compare
'kay, all green now :) |
f4d63d5
to
5759c16
Compare
Manual testing probably is good idea for this one? |
Yep, was going to create sttus-desktop PR with this. |
Here is the status-desktop PR status-im/status-desktop#15455 |
Let's ping @glitchminer and @qoqobolo 🙂 |
@chaitanyaprem thank you for the PR and pinging QA team. Could you please rebase status go branch to the latest develop and create status mobile PR as well? Also, would appreciate some details on what exactly has been affected and how should we test this PR from user perspective (what test scenarios/areas)? Thank you. |
4ae62d6
to
d59d451
Compare
Done and here it is status-im/status-mobile#20658. |
@chaitanyaprem I have left a comment in mobile PR, posting it here as well, so everyone is aware about the problem.
|
436ad54
to
2470244
Compare
Hey @cammellos! What do you think, is this a priority to test this PR on mobile or we can rely on e2e results from our side? Note that our e2e do not cover sharded communities. |
@pavloburykh thanks, given the priorities for 2.30, I don't think we have enough capacity in the QA team to test something that's not strictly needed for 2.30. If @chaitanyaprem thinks that this is not impacting anything unless the community is manually sharded, then e2e tests is probably good enough, but if it impacts production code, best to leave it for later |
afaik it doesn't affect any other code apart from sharded communities. But i'll want a yes from the great @richard-ramos as well to be sure. |
IMO it should be fine to merge this PR. The normal behavior of the app is maintained, and I think that as long as the custom shard RPC methods still work fine, which as far as we know, it is the case, we should be good to go! |
ad22714
to
dc8ea45
Compare
4853a00
to
4d84752
Compare
4d84752
to
f9ab393
Compare
2d2be5c
to
2b5cde1
Compare
Closes #5277