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

cli/discover: remove/add local collections if the remote collection is deleted/created #869

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dilyanpalauzov
Copy link
Contributor

This works when the destination backend is 'filesystem'.

-- add a new parameter to storage section:
implicit = ["create", "delete"]

Changes cli/utils.py:save_status(): when data is None, remove the underlaying file.

@lgtm-com
Copy link

lgtm-com bot commented Mar 7, 2021

This pull request introduces 3 alerts when merging a38bb96 into a42906b - view on LGTM.com

new alerts:

  • 2 for Except block handles 'BaseException'
  • 1 for Unused local variable

@lgtm-com
Copy link

lgtm-com bot commented Mar 7, 2021

This pull request introduces 2 alerts when merging 1c1cd3f into a42906b - view on LGTM.com

new alerts:

  • 2 for Except block handles 'BaseException'

@dilyanpalauzov
Copy link
Contributor Author

The tests fail with

    raise DeadlineExceeded(runtime, self.settings.deadline)
        hypothesis.errors.DeadlineExceeded: Test took 227.47ms, which exceeds the deadline of 200.00ms

@dilyanpalauzov
Copy link
Contributor Author

Will this get eventually integrated upstream?

@WhyNotHugo
Copy link
Member

@dilyanpalauzov Yup, sorry, just been busy, but the change makes sense. I'll deal with conflicts when I get a change :)

@dilyanpalauzov
Copy link
Contributor Author

Please elaborate what means “when I get a change”?

@WhyNotHugo
Copy link
Member

WhyNotHugo commented Jul 16, 2021 via email

…s deleted/created

This works when the destination backend is 'filesystem'.

-- add a new parameter to storage section:
   implicit = ["create", "delete"]

Changes cli/utils.py:save_status(): when data is None, remove the
underlaying file.
@WhyNotHugo
Copy link
Member

I've rebased your branch onto master and resolved all conflicts.

I'm a bit uncertain about how this works when you have:

  • storage A with implicit = "create"
  • storage B with implicit = "delete"

If there's a collection present in B, but missing in A, should it be deleted from B, or created in A?

@dilyanpalauzov
Copy link
Contributor Author

I have not thought on this case. I would say, the collection shall be created.

Rationale: I do not know why this combination is good for. Preventing data deletion in this unclear use-case is a good approach, as long as nobody proposes something better.

@dilyanpalauzov
Copy link
Contributor Author

Let me know if there is anything from my side, which shall be done for this PR. I do not understand the code of vdirsyncer very good.

@dilyanpalauzov
Copy link
Contributor Author

• storage A with implicit = "create"
• storage B with implicit = "delete"

If there's a collection present in B, but missing in A, should it be deleted from B, or created in A?

How about writing down,that the behaviour in this case is not defined.

@samsonjs
Copy link

Is there anything I can do to help with this one? I'd love to have this feature as well.

@samsonjs
Copy link

Perhaps only implicit creation should be allowed? That way there's never any accidental data loss, which is far worse than a zombie collection coming back from the dead. And there's no ambiguity about what to do in this scenario:

I'm a bit uncertain about how this works when you have:

storage A with implicit = "create"
storage B with implicit = "delete"

If there's a collection present in B, but missing in A, should it be deleted from B, or created in A?

Selfishly, implicit creation is the only part of this that I actually need so if eliminating implicit deletion helps move this forward at all here then that suits me just fine 😄

@dilyanpalauzov
Copy link
Contributor Author

I need also implicit deletion.

@samsonjs
Copy link

Fair enough. What about using conflict_resolution for this too? In the example scenario above there would be an error if none is specified, or if the config has "b wins" then the collection is deleted from A. I'm not sure if there's a good way use the diff command for this one though so maybe it could also produce an error when a command is configured.

I think that behaviour is actually pretty intuitive and most people would expect it to work that way.

@dilyanpalauzov
Copy link
Contributor Author

This pull request from me is stalled. I do use the changes on my system and it does work exactly as I want. I will appreciate any progress on this, but I am not going to modify the proposed changes.

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.

3 participants