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

Sync stuff #89

Merged
merged 10 commits into from
Jun 8, 2021
Merged

Sync stuff #89

merged 10 commits into from
Jun 8, 2021

Conversation

ErikBjare
Copy link
Member

@ErikBjare ErikBjare commented Feb 24, 2020

Resuming work from #71

What's missing:

  • "2-way" test with actual folder
  • Test for when events have been modified/deleted (when should things get synced?)
  • Use device IDs to identify remotes
    • Partially done, but can probably be done better.
  • refactor (some things can be broken down/reused)
  • better error handling (proper aw-client-rust errors, get rid of unwraps)
  • more comprehensive tests (should be >90% covered)
  • review processes/conventions
  • See comments in More progress on sync #71 and fix them
  • anything else?

@codecov
Copy link

codecov bot commented May 2, 2020

Codecov Report

Merging #89 (1f9071a) into master (6452957) will decrease coverage by 0.14%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #89      +/-   ##
==========================================
- Coverage   60.85%   60.70%   -0.15%     
==========================================
  Files          44       44              
  Lines        4989     5001      +12     
==========================================
  Hits         3036     3036              
- Misses       1953     1965      +12     
Impacted Files Coverage Δ
aw-server/src/main.rs 0.00% <0.00%> (ø)
aw-server/src/macros.rs 26.58% <0.00%> (-1.42%) ⬇️
aw-query/src/parser.rs 41.85% <0.00%> (-0.15%) ⬇️
aw-server/src/endpoints/mod.rs 78.04% <0.00%> (+2.43%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6452957...1f9071a. Read the comment docs.

@ErikBjare
Copy link
Member Author

ErikBjare commented May 2, 2020

Rebasing on master somehow closed the PR as there was no longer a diff. I assume the commit had been cherrypicked or something?

@ErikBjare
Copy link
Member Author

Got sync working using aw-client-rust. I've only tested syncing temporary databases to my local testing instance, but it seems to work!

@ErikBjare
Copy link
Member Author

For some reason can't request a review from @xylix, but would be nice to have as many eyes on this as possible.

@xylix
Copy link
Contributor

xylix commented May 2, 2020

Interesting that requesting doesn't work. I'll take a look around

@xylix
Copy link
Contributor

xylix commented May 2, 2020

The code looks pretty good. What are the currently missing features? Is "See comments in #71 and fix them" not complete yet?

@ErikBjare

This comment has been minimized.

@ErikBjare
Copy link
Member Author

@johan-bjareholt I've stumbled into an issue where I need to know the device ID for a remote bucket... Could you take a look at it so we can discuss options?

Copy link
Member

@johan-bjareholt johan-bjareholt left a comment

Choose a reason for hiding this comment

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

I've stumbled into an issue where I need to know the device ID for a remote bucket... Could you take a look at it so we can discuss options?

Maybe just have a migration where we add a "host-id" row to all buckets? Then we could add the hosts ID to the already existing buckets.

Later on we could have a table for users to set their own names for each host instead of having a uuid for each maybe? And possibly even use the new key_value API for that.

aw-server/src/config.rs Outdated Show resolved Hide resolved
aw-server/src/config.rs Outdated Show resolved Hide resolved
aw-sync/src/sync.rs Outdated Show resolved Hide resolved
@ErikBjare
Copy link
Member Author

@johan-bjareholt I think it's about time to merge this. There's work left but should be mergeable (assuming CI passes). Review plz.

aw-client-rust/src/lib.rs Show resolved Hide resolved
aw-server/Cargo.toml Show resolved Hide resolved
@ErikBjare
Copy link
Member Author

Merging this for now, as the CLI additions are needed for my PR in ErikBjare/quantifiedme#4

Will later continue the work in a new PR.

@ErikBjare ErikBjare merged commit 4a1943a into master Jun 8, 2021
@ErikBjare ErikBjare deleted the dev/cross-client-sync branch June 8, 2021 11:43
@ShootingKing-AM ShootingKing-AM mentioned this pull request Oct 13, 2022
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