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.

Sign up for GitHub

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

Jump to bottom

feat: remote-signal scenario #93

Merged
merged 18 commits into from
Oct 23, 2024
Merged

feat: remote-signal scenario #93

merged 18 commits into from
Oct 23, 2024

Conversation

neonphog
Copy link
Contributor

@neonphog neonphog commented Sep 12, 2024 •
edited
Loading

remote_signal

@neonphog neonphog marked this pull request as ready for review September 17, 2024 19:39
@neonphog neonphog marked this pull request as draft September 17, 2024 19:39
@neonphog neonphog marked this pull request as ready for review September 17, 2024 19:50
@neonphog neonphog changed the title wip: remote-signal scenario remote-signal scenario Sep 17, 2024
cdunster
cdunster reviewed Sep 18, 2024
View reviewed changes
scenarios/remote_signals/Cargo.toml Outdated Show resolved Hide resolved
scenarios/remote_signals/src/main.rs Show resolved Hide resolved
scenarios/remote_signals/src/main.rs Outdated Show resolved Hide resolved
@neonphog neonphog requested a review from cdunster September 18, 2024 17:29
cdunster
cdunster reviewed Sep 19, 2024
View reviewed changes
Copy link
Contributor

@cdunster cdunster left a comment

Choose a reason for hiding this comment

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

I think that now the scenario is called remote_signal_scenario so it needs to be changed in the CI 🙂

.github/workflows/performance.yaml Outdated Show resolved Hide resolved
.github/workflows/test.yaml Outdated Show resolved Hide resolved
.github/workflows/test.yaml Outdated Show resolved Hide resolved
@neonphog
Copy link
Contributor Author

haha, of course : ) must have been tired last night : )

ThetaSinner
ThetaSinner previously approved these changes Sep 24, 2024
View reviewed changes
Copy link
Member

@ThetaSinner ThetaSinner left a comment

Choose a reason for hiding this comment

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

Looking good! A couple of minor things

.github/workflows/test.yaml Outdated Show resolved Hide resolved
scenarios/remote_signals/README.md Outdated Show resolved Hide resolved
scenarios/remote_signals/src/main.rs Outdated Show resolved Hide resolved
scenarios/remote_signals/src/main.rs Show resolved Hide resolved
cdunster
cdunster previously approved these changes Oct 14, 2024
View reviewed changes
Copy link
Contributor

@cdunster cdunster left a comment

Choose a reason for hiding this comment

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

Small suggestion but LGTM

zomes/remote_signal/coordinator/Cargo.toml Show resolved Hide resolved
@neonphog neonphog dismissed stale reviews from cdunster and ThetaSinner via aca61fa October 14, 2024 20:06
cdunster
cdunster previously approved these changes Oct 15, 2024
View reviewed changes
scenarios/remote_signals/README.md Outdated Show resolved Hide resolved
cdunster
cdunster previously approved these changes Oct 16, 2024
View reviewed changes
ThetaSinner
ThetaSinner previously approved these changes Oct 17, 2024
View reviewed changes
scenarios/remote_signals/README.md Show resolved Hide resolved
@neonphog neonphog dismissed stale reviews from ThetaSinner and cdunster via 97b20b4 October 22, 2024 19:15
@neonphog neonphog changed the title remote-signal scenario feat: remote-signal scenario Oct 22, 2024
@neonphog
Copy link
Contributor Author

@cdunster @jost-s @ThetaSinner --

I'm getting another round of incompatibility with nix: https://github.com/holochain/wind-tunnel/actions/runs/11466978285/job/31908859553?pr=93

Can I just hand this over to one of you to see this through to the finish line at this point??

@jost-s
Copy link
Contributor

jost-s commented Oct 22, 2024

You didn't seem to have changed anything related. I'll take a look.

@cdunster
Copy link
Contributor

@neonphog and @jost-s, I'll take a look today as I have the same issue on my PR and @ThetaSinner commented that he solved it already on a separate PR:

Surprised we're seeing the arx issue without having upgraded that input. I just dealt with that on the binaries repo. It needs some tweaks to the derivation definition
#117 (comment)

cdunster
cdunster previously approved these changes Oct 23, 2024
View reviewed changes
@cdunster
Copy link
Contributor

LGTM. I merged the CI fix from main so now I can't approve this PR 😅

@ThetaSinner or @jost-s, do you want to have one last look over this so we can merge it?

@cdunster cdunster enabled auto-merge October 23, 2024 15:39
ThetaSinner
ThetaSinner previously approved these changes Oct 23, 2024
View reviewed changes
@cdunster cdunster added this pull request to the merge queue Oct 23, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to a conflict with the base branch Oct 23, 2024
@cdunster cdunster dismissed stale reviews from ThetaSinner and themself via 3bcae5e October 23, 2024 16:29
@cdunster cdunster enabled auto-merge October 23, 2024 16:30
neonphog and others added 18 commits October 23, 2024 18:38
@neonphog @cdunster
initial working remote-signal scenario
4ad6654
@neonphog @cdunster
better timeout tracking
e15c3e1
@neonphog @cdunster
ci
1bf5641
@neonphog @cdunster
lint
2adc493
@neonphog @cdunster
address code review comment
5ccf4a6
@neonphog @cdunster
remove 's' from scenario directory path
d034878
@neonphog @cdunster
woops, remove 's' from CI jobs as well
9e34986
@neonphog @cdunster
different name for zome vs scenario
995d1be
@neonphog @cdunster
Revert "different name for zome vs scenario"
1ed2c49 
This reverts commit 202332e.
@neonphog @cdunster
fix naming conflict by adding _scenario suffix
6eded25
@neonphog @cdunster
Apply suggestions from code review
8090a66 
Co-authored-by: Callum Dunster <[email protected]>
@jost-s @cdunster
fix: rename remote_signal_coordinator
31f6dcc
@jost-s @cdunster
refactor: rename scenario to remote_signals
0e14ffd
@neonphog @ThetaSinner @cdunster
Apply suggestions from code review
952c209 
Co-authored-by: ThetaSinner <[email protected]>
Co-authored-by: Callum Dunster <[email protected]>
@neonphog @cdunster
yaml fmt
6136037
@neonphog @cdunster
address code review comment
537ebfc
@neonphog @cdunster
Update scenarios/remote_signals/README.md
3fae307 
Co-authored-by: Callum Dunster <[email protected]>
@neonphog @cdunster
address code review comment
6a1b37a
@cdunster cdunster added this pull request to the merge queue Oct 23, 2024
Merged via the queue into main with commit 75536a6 Oct 23, 2024
2 checks passed
@cdunster cdunster deleted the rmt-sig branch October 23, 2024 18:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ThetaSinner ThetaSinner ThetaSinner approved these changes

@cdunster cdunster cdunster left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@neonphog @jost-s @cdunster @ThetaSinner