dynamicdan / sn-filesync

2-way sync ServiceNow field values to local files
MIT License
66 stars 37 forks source link

Upgrade to use standard npm/node package setup #21

Closed karimhernandez closed 8 years ago

karimhernandez commented 8 years ago

Hi Dan,

I wanted to start a dialogue with you, to gauge your interest regarding some changes to filesync. I've created a fork to experiment with some updates, many of which may be aligned with your roadmap for the project, but also contains some opinionated changes which, I admit, may not at all align.

In brief, I wanted to do away with dedicated binaries and custom shell scripts, to be able to run with my locally installed version of node (currently Node.js 0.12), and lastly make configs a little easier to use (incorporating patterns I've seen from config-handling in other command-line tools).

At your convenience, please review this fork and let me know if you might be interested in a PR containing any of these changes. Though arguably necessary, the restructuring done in my fork is in many ways quite radical. I figured some experimentation and posting an issue here was more appropriate than simply sending a pull request.

Tangentially related, I'm curious if you have a test script or perhaps other steps you personally take, in addition to utilizing the --test flag, to determine if a milestone is suitable for release. The changes I've made pass all the tests invoked by --test, but I'm skeptical that those alone can uncover any regressions or new bugs I may have introduced.

Thanks, -Karim

dynamicdan commented 8 years ago

Hi Karim,

Firstly, FANTASTIC! I'm always open to community support and improvements. FileSync also has a lot of room for these both feature wise and script/app architecture wise. It has a legacy of at least 2 major developers and some older concepts that should be improved. I've done a lot of refactoring but haven't found the time to iron out some issues like the dependencies on the older versions of node.

I like the idea of moving away from the "packaged install"/binary stuff. The issue was that not all technical consultants are fluent with setting up node and dev based environments so this was an easy way to speed up setup time. I think this could now be handled via a special readme section for install and setup. Using npm to install is of course a nice and standard solution. Note that some TCs/devs must work in highly locked down virtual clients that would not allow installing NPM/node so providing links to a packaged version may still be required.

Regarding --test, only core tests have been added for the purpose of avoiding data loss. Eg, it should not be possible to overwrite local files or server records in error. Any other errors like exporting the config file in error are considered not critical and therefore do not have tests behind them. The development approach in this regard is to stop future regression AFTER it happened once OR if it is a highly delicate piece of functionality that would need specific tests to be written.

The problem with writing tests is that they need to be maintained and this sucks up time. Feel free to add any tests you desire.

So moving forward, updating to the latest stable version of node and its dependencies is a good step which I would be keen to pull into this repo (if thoroughly tested). I would need a bit more time to review the actual commits (on holiday right now).

Cheers, D.

Echo3ToEcho7 commented 8 years ago

I wish that I had read this issue yesterday as I did the exact same thing :) My branch is here. I think that the only difference between what we did is that I moved app.js to bin/. Also, I am working off node 5.5 with no issues so far.

Personally, I was going to refactor the snc-client next to use restler instead of restify so that there is no need for a compiler to be installed to use filesync. I should be able to keep is segregated so that it would merge into the new layout.

karimhernandez commented 8 years ago

Great minds :)

I don't have a preference for either restify or restler, but alleviating the need for a compiler sounds like a good idea.

Currently I think your branch is a bit cleaner, since I also have changes related to configuration. I had planned to cherry-pick the binary deletion and relocation changes into a clean branch to submit for a PR, but that's essentially what your repo is :)

One general issue, npm package names with capital letters are considered invalid, and the name 'filesync' is already taken on npmjs.com. So if the package is to be installable by name, we'd need some alternative.

stegel commented 8 years ago

sn-filesync?

On Wed, Feb 3, 2016 at 2:23 PM karimhernandez notifications@github.com wrote:

Great minds :)

I don't have a preference for either restify or restler, but alleviating the need for a compiler sounds like a good idea.

Currently I think your branch is a bit cleaner, since I also have changes related to configuration. I had planned to cherry-pick the binary deletion and relocation changes into a clean branch to submit for a PR, but that's essentially what your repo is :)

One general issue, npm package names with capital letters are considered invalid, and the name 'filesync' is already taken on npmjs.com. So if the package is to be installable by name, we'd need some alternative.

— Reply to this email directly or view it on GitHub https://github.com/dynamicdan/filesync/issues/21#issuecomment-179413834.

Echo3ToEcho7 commented 8 years ago

That name works for me. We should probably let @dynamicdan decide when he gets back from holiday. The command line name could stay filesync or could be something else.

I also removed restify and added restler on my fork. It could use a little cleanup, but the tests are passing again. I will create a new issue to track the move to restler.

dynamicdan commented 8 years ago

Seems like my homework is piling up... I'll see if I can unblock some things.

In terms of a name, sn-filesync is ok. I was thinking about moving away from the FileSync name because of popular use and the disconnect to the utility of the tool. Eg record2filesync might make more sense. But I'm not the marketing genius. Any other ideas for names?

Or go web2.0 with something like snazilla? Syncarella? Synksy? Snaprdown (ServiceNow asynchronous partial record download?)

karimhernandez commented 8 years ago

recordsync makes the most sense to me.

In the silliness category, you could riff off of 'Heisenberg' and use some other character name from Breaking Bad. :)

dynamicdan commented 8 years ago

recordsync somewhat overlaps with the idea of syncing records between instances or keeping them in-sync. So not sure if that makes sense. FieldSync would of course be more accurate. I don't really like the sync idea though because we are just publishing and not really keeping things in sync (eg. instance can't sync to local files.. 1 way syncing). ScriptToInstance is probably the most accurate even though we can push any field type, script fields have the highest use case. Maybe RemoteScripting ?

Alas, I can't find a suitable name. BTW, I hate the Heisenberg theme. It let me down on the ess side.

karimhernandez commented 8 years ago

Yeah I didn't come up with a good name either. In my fork, I just named the executable servicenow. It's not like they'll ever branch out and do a proper SDK anytime soon.

And I didn't mean to imply I liked the Heisenberg theme - I'm constantly fighting with it and prototype for my ess work. I ended up just prettifying what was there and pulling it into my local project, so I could debug. But still, gustavo.js sounds cool :)

dynamicdan commented 8 years ago

Ok. Something simpler: sn-sync

Is it taken or confusing or something?

dynamicdan commented 8 years ago

FYI, the experimental branch has the pulled changes and is working so the readme just needs to be updated and a name decided on before I can merge it to master.

If you have time then please also switch to this branch to do further testing.

Next stop is updating to restler which is handled in #22

dynamicdan commented 8 years ago

Renamed to sn-filesync and published to npm repo.