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: switch wakuv2 to waku fleet #2519

Merged
merged 1 commit into from
Mar 21, 2024
Merged

chore: switch wakuv2 to waku fleet #2519

merged 1 commit into from
Mar 21, 2024

Conversation

yakimant
Copy link
Member

@yakimant yakimant commented Mar 7, 2024

No description provided.

Copy link
Collaborator

@Ivansete-status Ivansete-status left a comment

Choose a reason for hiding this comment

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

LGTM!

docs/contributors/continuous-integration.md Show resolved Hide resolved
@Ivansete-status Ivansete-status changed the title switch wakuv2 to waku fleet chore: switch wakuv2 to waku fleet Mar 7, 2024
@yakimant yakimant force-pushed the switch_wakuv2_to_waku branch 2 times, most recently from 2340991 to c28db6d Compare March 11, 2024 13:44
@yakimant yakimant marked this pull request as ready for review March 11, 2024 13:44
@yakimant yakimant force-pushed the switch_wakuv2_to_waku branch 2 times, most recently from 7bb06d5 to b01d495 Compare March 11, 2024 13:52
Copy link

github-actions bot commented Mar 11, 2024

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2519-rln-v2-false

Built from c8b2050

Copy link
Collaborator

@Ivansete-status Ivansete-status left a comment

Choose a reason for hiding this comment

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

LGTM! Great work! 💯

apps/chat2/chat2.nim Outdated Show resolved Hide resolved
examples/publisher.nim Outdated Show resolved Hide resolved
Copy link
Contributor

@SionoiS SionoiS left a comment

Choose a reason for hiding this comment

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

Very nice thanks!

Left some comments.

.github/ISSUE_TEMPLATE/prepare_release.md Outdated Show resolved Hide resolved
docs/tutorial/dingpu.md Show resolved Hide resolved
docs/tutorial/onchain-rln-relay-chat2.md Outdated Show resolved Hide resolved
docs/tutorial/rln-chat2-live-testnet.md Outdated Show resolved Hide resolved
Copy link
Contributor

@NagyZoltanPeter NagyZoltanPeter left a comment

Choose a reason for hiding this comment

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

Approve with little nitpick.
..and a question: While we have this https://fleets.status.im/ but wouldn't it be possible to store all these hard coded addresses in a git repository with some script could be used to retrieve the necessary set of addresses and store in env variables to be used within our apps?
Such solution would give flexibility and automation for using the necessary fleet without such hard coding.
WDYT? cc:@SionoiS @Ivansete-status @gabrielmer @jm-clius

.github/ISSUE_TEMPLATE/prepare_release.md Outdated Show resolved Hide resolved
@yakimant
Copy link
Member Author

@NagyZoltanPeter, I admire your idea to extract hardcoded links and use https://fleets.status.im

Status Desktop and Mobile already do it. They store a copy of json in repo and update it with make target.

But this is out of the scope of this PR, I just want to replace the old references.
Although this PR can be useful for future work.

@yakimant yakimant closed this Mar 19, 2024
@yakimant yakimant reopened this Mar 19, 2024
@yakimant yakimant force-pushed the switch_wakuv2_to_waku branch 3 times, most recently from fed06c0 to b609ec5 Compare March 19, 2024 14:09
@yakimant
Copy link
Member Author

@SionoiS, @NagyZoltanPeter, @Ivansete-status, some CI jobs are failing.
Was not trivial to figure out, what happened, can you plase help?

@SionoiS
Copy link
Contributor

SionoiS commented Mar 19, 2024

@SionoiS, @NagyZoltanPeter, @Ivansete-status, some CI jobs are failing. Was not trivial to figure out, what happened, can you plase help?

At first glace -> /home/runner/work/nwaku/nwaku/examples/publisher.nim(33, 3) Error: closing " expected

@NagyZoltanPeter
Copy link
Contributor

@SionoiS, @NagyZoltanPeter, @Ivansete-status, some CI jobs are failing. Was not trivial to figure out, what happened, can you plase help?

At first glace -> /home/runner/work/nwaku/nwaku/examples/publisher.nim(33, 3) Error: closing " expected

@yakimant: Yes, @SionoiS is right. In commit: b609ec5 there is a missing closing apostrophe.
But I see there are other issues with it.

First of all it will be hard to merge in this way due to the nph formatting recently applied to the complete nwaku nim code base.
I can help with it if you need!

@NagyZoltanPeter
Copy link
Contributor

@SionoiS, @NagyZoltanPeter, @Ivansete-status, some CI jobs are failing. Was not trivial to figure out, what happened, can you plase help?

At first glace -> /home/runner/work/nwaku/nwaku/examples/publisher.nim(33, 3) Error: closing " expected

@yakimant: Yes, @SionoiS is right. In commit: b609ec5 there is a missing closing apostrophe. But I see there are other issues with it.

First of all it will be hard to merge in this way due to the nph formatting recently applied to the complete nwaku nim code base. I can help with it if you need!

@yakimant: Did you start a rebase to latest origin/master? If so it has not been finished.
I fixed the code, will push it here to see if ci will go well.

@NagyZoltanPeter
Copy link
Contributor

@yakimant, @SionoiS : I pushed fixes for the code and test. Tests seems running well... but not perfect.
ubuntu-latest-rln-v2 fails with port is already occupied that test would use for REST API.
Other tests just seems run out of time.... Any thoughts on it?

@yakimant
Copy link
Member Author

@NagyZoltanPeter, thanks for fixing my stupid mistakes!

As for tests/test_waku_enr.nim - maybe let's change the ENR instead?
I think it would be nice to get rid of wakuv2 references at all.

@NagyZoltanPeter
Copy link
Contributor

@NagyZoltanPeter, thanks for fixing my stupid mistakes!

As for tests/test_waku_enr.nim - maybe let's change the ENR instead? I think it would be nice to get rid of wakuv2 references at all.

Surely, but I did not know what exactly would be the correct expected result, while the test was just about to decode ENR properly.

@SionoiS
Copy link
Contributor

SionoiS commented Mar 20, 2024

@yakimant, @SionoiS : I pushed fixes for the code and test. Tests seems running well... but not perfect. ubuntu-latest-rln-v2 fails with port is already occupied that test would use for REST API. Other tests just seems run out of time.... Any thoughts on it?

I launched a re-run. Seams like flaky tests except, 1 test in the js interop failed with a shard error.
Maybe sharding info is wrong in some ENR?

@NagyZoltanPeter
Copy link
Contributor

Seems much better now.
Only optional js-waku-node tests fails with timout.

 Peer Exchange
    Auto Discovery
      1) should discover peers other than used for bootstrapping on test fleet
      ✔ should discover peers other than used for bootstrapping on prod fleet (1566ms)


  6 passing (4m)
  1 failing

  1) Peer Exchange
       Auto Discovery
         should discover peers other than used for bootstrapping on test fleet:
     Error: Timeout of 50000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/home/runner/work/nwaku/nwaku/packages/tests/tests/peer-exchange/pe.optional.spec.ts)
      at listOnTimeout (node:internal/timers:569:17)
      at processTimers (node:internal/timers:512:7)

As this error exists in general for other PR's CI jobs, @yakimant I think you can now merge this change!!!
Thank you for it.

@yakimant yakimant force-pushed the switch_wakuv2_to_waku branch 2 times, most recently from 2b630d4 to 18a0535 Compare March 20, 2024 15:28
@yakimant yakimant merged commit 18a0535 into master Mar 21, 2024
14 of 15 checks passed
@yakimant yakimant deleted the switch_wakuv2_to_waku branch March 21, 2024 11:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants