-
-
Notifications
You must be signed in to change notification settings - Fork 449
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
Selective sync #834
Selective sync #834
Conversation
ee25724
to
2c6d311
Compare
Sweet! This looks good. The only concern I have is that this is stateless. If you run:
Every time you want to update
Then it'll download everything. In my mind this was a blocker before with the selective sync. Based on your conversation with SDSS folks, do you think we should be worried about this? We could make it stateful by adding a |
@Karissa What if you want to sync a remote file named |
src/parse-files.js
Outdated
|
||
try { | ||
if (fs.statSync(input).isFile()) { | ||
parsed = fs.readFileSync(input).toString().trim().split('\n') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this mean problems for files on windows ending with \r\n
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing it to .split(/\r?\n/)
would prob work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point!
I would split the functionality into two different arguments, e.g. |
Want this feature so bad :) I think we should use Beaker style filters |
src/lib/selective-sync.js
Outdated
debug('archive update') | ||
bus.emit('render') | ||
}) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm gonna steal most of this and put in hyperdrive later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice
In general this looks solid 👍 👍 👍 |
@ralphtheninja yeah i think you bring up a good point, but I agree with @joehand that perhaps having some state file (like |
OK I made some changes so now:
TODO in a different PR:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.datdownload
file stuff looks good. Last questions:
- Should we read/check that file for the regular
dat sync
anddat pull
commands, so we avoid accidental downloads? EDIT: misread the code and this works fordat sync
already ✨ - Do we want to write to
.datdownload
when you pass--select=file.txt,etc.
?
package.json
Outdated
@@ -1,6 +1,6 @@ | |||
{ | |||
"name": "dat", | |||
"version": "13.7.0", | |||
"version": "13.8.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm gonna try to get #828 released for 13.8 too, can we increment version separate from the PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
src/commands/clone.js
Outdated
@@ -8,6 +8,11 @@ module.exports = { | |||
].join('\n'), | |||
options: [ | |||
{ | |||
name: 'empty', | |||
default: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add boolean setting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
got it
src/lib/selective-sync.js
Outdated
archive.readdir(dirname, function (err, entries) { | ||
if (err) return bus.emit('exit:error', err) | ||
entries.forEach(function (entry) { | ||
emitter.emit('download', path.join(dirname, entry)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whats the benefit here of using EventEmitter instead of calling download(entry)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea was we could also figure out when it was done and call emitter.emit('end')
.. right now it just stays open and you have to watch the download speed
src/lib/selective-sync.js
Outdated
function download (entry) { | ||
debug('selected', entry) | ||
archive.stat(entry, function (err, stat) { | ||
if (err) return bus.emit('exit:error', err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be nice if we can warn without exiting if they select a file that doesn't exist. But that may be a harder UI problem we can solve later.
src/commands/sync.js
Outdated
name: 'selectFromFile', | ||
boolean: false, | ||
default: false, | ||
help: 'Sync only the list of selected files or directories in the given file. (default: .datdownload)', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do I use the default? I thought dat sync --selectFromFile
would use .datdownload
by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then tried this:
❯ dat sync --select
/Users/joe/code/node_modules/dat/src/commands/sync.js:80
if (opts.select) opts.selectedFiles = opts.select.split(',')
^
TypeError: opts.select.split is not a function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you just use dat sync
and it'll use .datdownload by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lol @ me trying to be too fancy 🤦♂️
It'd be cool to put a message after
|
@@ -14,6 +15,7 @@ module.exports = function (state, bus) { | |||
if (state.opts.http) serve(state, bus) | |||
|
|||
if (state.writable && state.opts.import) doImport(state, bus) | |||
else if (state.opts.sparse) selectiveSync(state, bus) | |||
else download(state, bus) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be cool to have a download progress UI =). I think we can update the dat-node/stats to work for this but that can wait.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, maybe lets open a PR on that
@joehand It already gives that message on clone. Or what kind of message were you thinking? |
Ya just some more instruction on how to use the selective stuff:
Part of the issue was that |
src/commands/sync.js
Outdated
@@ -21,6 +21,19 @@ module.exports = { | |||
abbr: 'ignore-hidden' | |||
}, | |||
{ | |||
name: 'selectFromFile', | |||
boolean: false, | |||
default: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we set the default here? The --help
prints out the default, so this was why I got confused:
dat sync --help
...
--selectFromFile, -select-from-file
Sync only the list of selected files or directories in the given file. (default: .datdownload) (default: false)
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh i didn't realize that. yeah sure
4403259
to
ec7037e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✨
selective UI for progress ,when complete ? |
Adds selective sync. This is a workflow requested by the Sloan Digital Sky Survey researchers at Berkeley Lawrence Lab.
New features added:
dat clone <key> --empty
: will create a sparse archive and won't download any files by default.dat sync <dir> --select=<remote-file>,<remote-file>,<remote-file>
: will download remote files with the selected namesOR
dat sync <dir> --select-from-file=<local-file-list.txt>
will parse the file and download all files listed (separated by newlines). Default is.datdownload
For example, check out this short video for a demo: https://www.dropbox.com/s/ih0pu1wqt2c1h9b/selective-sync-demo.mov?dl=0
Things this PR doesn't do yet: