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: leave and drop docs #1589

Merged
merged 12 commits into from
Oct 10, 2023
Merged

feat: leave and drop docs #1589

merged 12 commits into from
Oct 10, 2023

Conversation

Frando
Copy link
Member

@Frando Frando commented Oct 6, 2023 •
edited
Loading

Description

In iroh-sync:

  • feat: Add Store::remove_replica to remove a replica (namespace secret key and all entries)
  • feat: Adds closed atomic bool on Replica to ensure no operations can be performed on closed or removed replicas
  • refactor: To not add yet another Arc for the closed marker, move all things into InnerReplica

In iroh:

  • feat: Add DocDrop to RPC protocol
  • rename StopSync to Leave
  • feat: Add doc leave command to CLI / console. doc leave runs DocStopSync in the RPC layer (we already had that),
  • feat: Add doc drop command to delete a document

Notes & open questions

Fixes #1497

Change checklist

  • Self-review.
  • Documentation updates if relevant.
  • Tests if relevant.

This was referenced Oct 6, 2023
Closed
Closed
dignifiedquire
dignifiedquire reviewed Oct 6, 2023
View reviewed changes
Copy link
Contributor

@dignifiedquire dignifiedquire left a comment

Choose a reason for hiding this comment

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

looks good, other than the naming of the console command. Especially because it is inconsistent with the naming in client.rs

@Frando
Copy link
Member Author

Frando commented Oct 6, 2023 •
edited
Loading

I adjusted the naming to be leave everywhere for consistency.
But I also agree that maybe two ops (leave, and remove or delete) might be better.
leave and leave --remove was suggested by @b5 in #1460.
So we need a decision :-) Then I'll adjust the PR for whatever our outcome is. Would be good to keep client and cli aligned here I think.

Latest commit adds a high-level test and also makes sure that the event subscription streams are properly terminated for removed docs.

@Frando Frando changed the title feat: leave and remove docs feat: leave and drop docs Oct 6, 2023
@Frando
Copy link
Member Author

Frando commented Oct 6, 2023

Ok we decided to drop documents. Did the renames in the recent commits.

dignifiedquire
dignifiedquire reviewed Oct 9, 2023
View reviewed changes
iroh/src/commands/sync.rs Outdated
Self::Drop { doc } => {
let doc = get_doc(iroh, env, doc).await?;
iroh.docs.drop_doc(doc.id()).await?;
println!("Doc {} has been removed.", fmt_short(doc.id()));
Copy link
Contributor

Choose a reason for hiding this comment

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

what about asking for confirmation?

@Frando
Copy link
Member Author

Frando commented Oct 9, 2023

Added a confirmation prompt.

dignifiedquire
dignifiedquire reviewed Oct 9, 2023
View reviewed changes
iroh/Cargo.toml Outdated Show resolved Hide resolved
@Frando Frando added this pull request to the merge queue Oct 10, 2023
Merged via the queue into main with commit d7a3dd3 Oct 10, 2023
15 checks passed
@dignifiedquire dignifiedquire deleted the doc-leave-remove branch November 1, 2023 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dignifiedquire dignifiedquire dignifiedquire approved these changes

Assignees
No one assigned
Labels
None yet
Projects
iroh
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add an iroh doc leave command
2 participants
@Frando @dignifiedquire