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

feat(rln-relay): removed rln from experimental 🚀 #2001

Merged
merged 3 commits into from
Sep 11, 2023
Merged

Conversation

rymnc
Copy link
Contributor

@rymnc rymnc commented Sep 7, 2023

Description

This PR removes rln from the experimental feature set, as the acceptance criteria for #1906

Changes

  • updated makefile targets to make rln compiled by default
  • removed all references to when defined(rln)
  • updated github action workflow manifests

note: i left in the EXPERIMENTAL flag for any future protocol that will be experimental, it seems to
make sense to leave it in instead of removing it and then adding it back in when its needed, but open to
opinions

Removed the EXPERIMENTAL flag, and all its usages

How to test

  1. Build wakunode2 as usual, rln will be included make -j12 wakunode2
  2. Run tests as usual, rln tests will run too make -j12 test

Issue

closes #1906

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

had to be deleted - actions was acting out
@vpavlin is there a way to leave the file in but disable the workflow?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we can manually disable workflows through GH actions UI

@rymnc rymnc self-assigned this Sep 7, 2023
@github-actions
Copy link

github-actions bot commented Sep 7, 2023

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2001

Built from a4839d1

@rymnc rymnc marked this pull request as ready for review September 8, 2023 12:14
Copy link
Contributor

@alrevuelta alrevuelta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm! left a comment.

if node.wakuRlnRelay == nil:
return false
return await node.wakuRlnRelay.isReady()
if node.wakuRlnRelay == nil:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mm with this a node that doesnt have rln enabled will never be ready. perhaps we should add the return true back?
but yeah, problem is, if wakuRlnRelay=nil is it because i) rln is not enabled and we are ready or because ii) rln is enabled but not mounted and in this case we are not ready.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wakuRlnRelay is nil whenever the --rln-relay flag is not passed in

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 88bfe6e

Copy link
Member

@vpavlin vpavlin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM apart from the @alrevuelta's comment on isReady:) But I don't see any "right and quick" solution

@rymnc
Copy link
Contributor Author

rymnc commented Sep 8, 2023

@NagyZoltanPeter would you maybe know why the ci failed on this test?

2023-09-08T13:28:03.3911193Z �[96mINF�[0m 2023-09-08 13:28:03.389+00:00 �[1mstopping rln-relay                        �[0m topics="�[93mwaku rln_relay�[0m" �[96mtid�[0m=�[94m10538�[0m �[96mfile�[0m=�[94mrln_relay.nim:88�[0m

@NagyZoltanPeter
Copy link
Contributor

@NagyZoltanPeter would you maybe know why the ci failed on this test?

2023-09-08T13:28:03.3911193Z �[96mINF�[0m 2023-09-08 13:28:03.389+00:00 �[1mstopping rln-relay                        �[0m topics="�[93mwaku rln_relay�[0m" �[96mtid�[0m=�[94m10538�[0m �[96mfile�[0m=�[94mrln_relay.nim:88�[0m

Ah .... Actually I should have foresee this.
I was testing fake m false case with not having waku rln not mounted.
So it leads to that we need a more sophisticated isReady as we discussed already. Due we need to check against config what happened and what state we are.

For now I would suggest remove failure case from test_rest_health to quick fix it for now.

I will add a new issue to track this further.

@rymnc rymnc merged commit 645b034 into master Sep 11, 2023
16 checks passed
@rymnc rymnc deleted the rln-non-experimental branch September 11, 2023 06:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

chore(rln-relay): Requirements to consider RLN ready (non experimental)
5 participants