Skip to content

feat: disable triggerBigSync #84

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
Open
laggingreflex wants to merge 7 commits into
base: master
Choose a base branch
from
Open

feat: disable triggerBigSync #84

laggingreflex wants to merge 7 commits into from

Conversation

@laggingreflex
Copy link
Contributor

@laggingreflex laggingreflex commented Feb 21, 2017

Lets you add an option "triggerBigSync": false in project conf to disable doing a big rsync.

fixes #79

@coveralls
Copy link

coveralls commented Feb 21, 2017

Coverage Status

Coverage increased (+0.04%) to 97.357% when pulling 76b5f7d on laggingreflex:feat/disable-bigSync into 00dc1a8 on appnexus:master.

joelgriffith
joelgriffith suggested changes Feb 21, 2017
View reviewed changes
Copy link
Contributor

@joelgriffith joelgriffith left a comment

Choose a reason for hiding this comment

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

Since you're adding a new field to the project configuration, could also add the appropriate prompt when setting up a new project? Should be in: https://github.com/appnexus/sicksync/blob/master/src/project-helper.js#L18

src/local/index.js Outdated
'encryption'
);
});
if (projectConf.triggerBigSync !== false) {
Copy link
Contributor

@joelgriffith joelgriffith Feb 21, 2017

Choose a reason for hiding this comment

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

Can probably just do if (projectConf.triggerBigSync) {. Older configurations won't have this definition, and will returnundefined, which is falsy.

Copy link
Contributor Author

@laggingreflex laggingreflex Feb 21, 2017

Choose a reason for hiding this comment

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

I'm checking for not false. If older config has it undefined then this would be true which is the current default behavior. if (projectConf.triggerBigSync) { would disable it for all older configs that have it undefined.

Copy link
Contributor Author

@laggingreflex laggingreflex Feb 21, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Should it better be labeled as something like disableRsync which would be totally new option and not break any previous config?

@laggingreflex
Copy link
Contributor Author

laggingreflex commented Feb 21, 2017

I've updated it to be a --disable-rsync cli arg and disableRsync as project config

I've also update the --no-delete cli arg I added earlier to be --disable-deletion cli arg and disableDeleteion project arg

@coveralls
Copy link

coveralls commented Feb 21, 2017

Coverage Status

Coverage decreased (-0.4%) to 96.957% when pulling bbf9754 on laggingreflex:feat/disable-bigSync into 00dc1a8 on appnexus:master.

@laggingreflex
Copy link
Contributor Author

laggingreflex commented Feb 21, 2017

I'm currently testing it out and having some issues.

Don't merge it yet, I'll let you know when. Sorry for hasty commits.

@coveralls
Copy link

coveralls commented Feb 21, 2017

Coverage Status

Coverage decreased (-0.4%) to 96.957% when pulling 5e1635b on laggingreflex:feat/disable-bigSync into 00dc1a8 on appnexus:master.

@coveralls
Copy link

coveralls commented Feb 21, 2017

Coverage Status

Coverage decreased (-0.4%) to 96.97% when pulling b861ede on laggingreflex:feat/disable-bigSync into 00dc1a8 on appnexus:master.

@laggingreflex
Copy link
Contributor Author

laggingreflex commented Feb 21, 2017 •
edited
Loading

I was forgetting about a particular callback which was causing the issue. Wrapping triggerBigSync in an if just like that was not a great idea:

if(..) {
  triggerBigSync(projectConf, { debug: config.debug }, () => {
    localLog(text.SYNC_ON_LARGE_CHANGE_DONE);
    fsHelper.watch();
  });
}

There's a callback there which wouldn't get called.

I've used async/await now (added a babel-preset-stage-0) with which it could be solved and simplified:

if (...) {
  await triggerBigSync(projectConf, { debug: config.debug });
}
localLog(text.SYNC_ON_LARGE_CHANGE_DONE);
fsHelper.watch();

Although it alters the API a bit (triggerBigSync now returns a promise) so some tests might be failing. I'll see if I can work on those later in the week. Feel free to review now.

@laggingreflex
feat: disable triggerBigSync
e94c1b3 
Lets you add an option `"triggerBigSync": false` in project conf to disable doing a big rsync.
@laggingreflex
fix: Disable deletion
ed03866 
Update the logic that controlled disabling deletion from just a --no-delete argument to --disable-deletion argument as well as "disableDeleteion" project config
@laggingreflex
fix: disableRsync (arg * project conf) instead of project.triggerBigS…
acb156a 
…ync project conf
@laggingreflex
fix: projectConf.disableRsync
ab1e3eb
@laggingreflex
babel-preset-stage-0 for async/await
5195de6
@coveralls
Copy link

coveralls commented Feb 21, 2017

Coverage Status

Coverage increased (+0.05%) to 96.97% when pulling 0a7dffa on laggingreflex:feat/disable-bigSync into af55b54 on appnexus:master.

joelgriffith
joelgriffith reviewed Feb 21, 2017
View reviewed changes
src/big-sync.js
rsync.progress();
rsync.output(consoleLogFromBuffer, consoleLogFromBuffer);
}

Copy link
Contributor

@joelgriffith joelgriffith Feb 21, 2017

Choose a reason for hiding this comment

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

nice!

@joelgriffith
Copy link
Contributor

joelgriffith commented Feb 21, 2017

This looks great, we'll see what the CI finds test wise.

@coveralls
Copy link

coveralls commented Feb 21, 2017

Coverage Status

Coverage increased (+0.05%) to 96.97% when pulling 0a7dffa on laggingreflex:feat/disable-bigSync into af55b54 on appnexus:master.

@coveralls
Copy link

coveralls commented Feb 22, 2017

Coverage Status

Coverage increased (+0.05%) to 96.97% when pulling 6165fbf on laggingreflex:feat/disable-bigSync into af55b54 on appnexus:master.

@laggingreflex laggingreflex mentioned this pull request Feb 22, 2017
Open
@joelgriffith
Copy link
Contributor

joelgriffith commented Feb 28, 2017

I just pushed some updates to get tests running (properly) again, sorry for the thrash. I can update your PR to resolve the conflicts here on Friday.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@joelgriffith joelgriffith joelgriffith approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

An option to disable rsync

3 participants

@laggingreflex @coveralls @joelgriffith