These are chat archives for deployd/contributors

30th
Jan 2015
Eric Fong
@ericfong
Jan 30 2015 07:35

On the means time shall we start 0.8.1 branch ?

BTW, really hope can launch 0.8 sooner.

#494 is ok to test and rebase ?

Nicolas Ritouet
@NicolasRitouet
Jan 30 2015 07:36
Well, it doesn't make sense to have a 0.8.x branch yet
0.8.x will contain bug fixes from 0.8
New feature will be in a 0.9 branch
Nicolas Ritouet
@NicolasRitouet
Jan 30 2015 07:42
@eric: I'll assign you some pr
Eric Fong
@ericfong
Jan 30 2015 07:43
OK
test & rebasing 479
Nicolas Ritouet
@NicolasRitouet
Jan 30 2015 08:57
479 ?
Eric Fong
@ericfong
Jan 30 2015 09:11
497
Nicolas Ritouet
@NicolasRitouet
Jan 30 2015 10:15
@0ff @andreialecu what’s the status of this: deployd/deployd#494
?
Andrei Alecu
@andreialecu
Jan 30 2015 10:31
I sent @0ff a PR with some new changes yesterday but I don't know if he seen it
hey @0ff , check that out when you get a chance :)
Andrei Alecu
@andreialecu
Jan 30 2015 11:10
and.. by the way, and I still don't understand why you won't take this: deployd/deployd#476 would've saved several hours for me so far not having to switch back and forth between running unit tests and integration tests when I prepared PRs
Nicolas Ritouet
@NicolasRitouet
Jan 30 2015 11:37
because I’m still searching for a better idea
for example
supertest (https://github.com/tj/supertest) takes an express application as param, start it and stop it
hoodie also has a different approach
Nicolas Ritouet
@NicolasRitouet
Jan 30 2015 11:43
again, changing the API signature has lots of consequences, I want to do it only if necessary
I’m sorry it causes you some extra work
Nicolas Ritouet
@NicolasRitouet
Jan 30 2015 12:08
@0ff are you from Berlin?
Nicolas Ritouet
@NicolasRitouet
Jan 30 2015 13:38
is it me or Resource is loaded but not used: https://github.com/deployd/deployd/blob/master/lib/server.js#L5 ?
Andrei Alecu
@andreialecu
Jan 30 2015 15:18
I was on mobile didn't have access to gitter
at PC now
Nicolas Ritouet
@NicolasRitouet
Jan 30 2015 15:18
ok :)
Andrei Alecu
@andreialecu
Jan 30 2015 15:18
I really don't understand what you mean by changing the API signature
the CLI is not the api
Nicolas Ritouet
@NicolasRitouet
Jan 30 2015 15:18
adding a param to the CLI
it’s an API
if we add this, we can’t remove it
it’s a contract between us and the users
anyway, we’re not the first one who needs this kind of features
and I’m sure we can find a good alternative
we both want the same result
Andrei Alecu
@andreialecu
Jan 30 2015 15:20
the alternative will be repeating code
and the CLI is only useful at first when you're starting with deployd
I think we should migrate away from it
for 0.10 maybe
Nicolas Ritouet
@NicolasRitouet
Jan 30 2015 15:20
our priority with Deployd is to allow beginners to build api
Andrei Alecu
@andreialecu
Jan 30 2015 15:20
the template created by the CLI should be an app.js
Nicolas Ritouet
@NicolasRitouet
Jan 30 2015 15:20
the CLI helps a lot for that
Andrei Alecu
@andreialecu
Jan 30 2015 15:20
that starts the server
which you can run via node app.js or something to that effect
something standard
Nicolas Ritouet
@NicolasRitouet
Jan 30 2015 15:22
actually, all of this really depends on the feedback after the release
if we have some feedbacks, people will tell us what they would like
Andrei Alecu
@andreialecu
Jan 30 2015 15:23
personally I think appealing to beginners with the current deployd is a horrible idea, mainly because there is no default security and a beginner won't understand he needs to secure every single event
Nicolas Ritouet
@NicolasRitouet
Jan 30 2015 15:23
maybe they don’t care about the CLI
I personnaly don’t use the CLI
but I recommended deployd to some frontend dev and they were able to build awesome project based on deployd using the CLI
Andrei Alecu
@andreialecu
Jan 30 2015 15:24
sure, it's easy to build something apparently great
but how secure is it? there are so many leaks in deployd that I can't use it seriously until I plug them
like collection:changed events being fired everywhere for everything, and anyone can see them
all sort of sidechannel attacks are possible
I think deployd is far from being a mature API right now, should be considered alpha
Nicolas Ritouet
@NicolasRitouet
Jan 30 2015 15:27
true
Andrei Alecu
@andreialecu
Jan 30 2015 15:27
and other things like dpd.js exposing all collections regardless of whether the client even uses most of them or they should be considered internal
we should seriously have a big warning everywhere that security is not provided by default and you're reponsible for securing the endpoints/event scripts
this is bugging me the most
Nicolas Ritouet
@NicolasRitouet
Jan 30 2015 15:29
why is it bugging you? people should be aware of the tools they use
Andrei Alecu
@andreialecu
Jan 30 2015 15:30
I don't want to be responsible for creating something that beginners will use which can so easily be abused by others
Nicolas Ritouet
@NicolasRitouet
Jan 30 2015 15:30
nobody is responsible here, it’s an open source project
we provide some code, they can do whatever they want with it
Andrei Alecu
@andreialecu
Jan 30 2015 15:30
well, right, just saying that there should be a warning
Nicolas Ritouet
@NicolasRitouet
Jan 30 2015 15:30
not a bad idea
Andrei Alecu
@andreialecu
Jan 30 2015 15:30
if the goal is to appeal to beginners like you said
Nicolas Ritouet
@NicolasRitouet
Jan 30 2015 15:31
but I hope that we’ll be secure enough not to have these warnings some day
Andrei Alecu
@andreialecu
Jan 30 2015 15:31
beginners won't understand that you can easily get their collections from dpd.js and issue a DELETE to their collection with { id: { $ne: null } } and wipe out their database
Nicolas Ritouet
@NicolasRitouet
Jan 30 2015 15:31
we’ll fix the security issues one by one
Andrei Alecu
@andreialecu
Jan 30 2015 15:32
this is not necessarily a security issue as it is a need for them to proactively script it in the DELETE event
Nicolas Ritouet
@NicolasRitouet
Jan 30 2015 15:32
what are you actually suggesting? that deployd shoulnd’t be for beginners?
Andrei Alecu
@andreialecu
Jan 30 2015 15:32
no, it can be
but there needs to be better documentation right in the tooling
Nicolas Ritouet
@NicolasRitouet
Jan 30 2015 15:33
ok
I keep that in mind
Andrei Alecu
@andreialecu
Jan 30 2015 15:33
in the dashboard for example, or even prepopulate all scripts
Nicolas Ritouet
@NicolasRitouet
Jan 30 2015 15:33
interesting ...
Andrei Alecu
@andreialecu
Jan 30 2015 15:33
with things like cancel("Please secure this before removing this warning")
Nicolas Ritouet
@NicolasRitouet
Jan 30 2015 15:33
indeed, that would be nice
Andrei Alecu
@andreialecu
Jan 30 2015 15:35
so regarding that PR with npm test
the integration tests can be easily made to use the same mongo server as the unit tests
which would solve the need for starting mongo
but then they wouldn't really be integration tests because the integration isdpd
the CLI
at least that's what it's advertised to be in the docs and on the site, even if I personally don't use it
Andrei Alecu
@andreialecu
Jan 30 2015 20:27
@NicolasRitouet I reworked that PR to not change the CLI, feel free to look at it again
so we can finally get it over with if possible :)
Nicolas Ritouet
@NicolasRitouet
Jan 30 2015 20:30
I’m taking a look right now
Andrei Alecu
@andreialecu
Jan 30 2015 20:31
I just force pushed a minor comment removal
make sure you have latest
Nicolas Ritouet
@NicolasRitouet
Jan 30 2015 20:31
ok
didn’t you send a PR to add a param to remove the error message?
indeed
metaskills/mocha-phantomjs#169
Andrei Alecu
@andreialecu
Jan 30 2015 20:38
They added it
Nicolas Ritouet
@NicolasRitouet
Jan 30 2015 20:38
but it’s not release yet, right?
should we use master or just wait?
Andrei Alecu
@andreialecu
Jan 30 2015 20:39
Yes
Could use master if you want
Nicolas Ritouet
@NicolasRitouet
Jan 30 2015 20:40
you decide
should I wait for a new push?
or you’re good?
because I’m about to rebase :)
Andrei Alecu
@andreialecu
Jan 30 2015 20:45
I'll change to master and add the flag to hide those errors one sec
Nicolas Ritouet
@NicolasRitouet
Jan 30 2015 20:45
oki
Andrei Alecu
@andreialecu
Jan 30 2015 20:58
Done, it got messy because I did this from my phone :)
Nicolas Ritouet
@NicolasRitouet
Jan 30 2015 20:58
lol
Andrei Alecu
@andreialecu
Jan 30 2015 20:58
Stepped away from computer
Yeah, it was a challenge.
Should be done now :)
Nicolas Ritouet
@NicolasRitouet
Jan 30 2015 21:09
weird, I have some errors on my machine when I run the tests again
but it works on travis ci
Andrei Alecu
@andreialecu
Jan 30 2015 21:10
What are they?
Nicolas Ritouet
@NicolasRitouet
Jan 30 2015 21:10
apparently, dpd is already running (port are used)
proc.stdout.on('data', function(data) {
^
TypeError: Cannot call method 'on' of null
Andrei Alecu
@andreialecu
Jan 30 2015 21:11
Do they happen every time?
Nicolas Ritouet
@NicolasRitouet
Jan 30 2015 21:11
now yes, but it worked the first time
Andrei Alecu
@andreialecu
Jan 30 2015 21:14
Works fine on Windows, I haven't tried on my Mac
Maybe there's something different on Mac
Nicolas Ritouet
@NicolasRitouet
Jan 30 2015 21:15
it’s proc.stdout which is null
from the dpd fork
Andrei Alecu
@andreialecu
Jan 30 2015 21:16
How are you running?
Nicolas Ritouet
@NicolasRitouet
Jan 30 2015 21:16
npm test
Andrei Alecu
@andreialecu
Jan 30 2015 21:16
npm test?
Maybe a current directory issue?
Seems like it can't find dpd or something similar for that to be null
Nicolas Ritouet
@NicolasRitouet
Jan 30 2015 21:18
I’m restarting to make sure I have a clean env
Andrei Alecu
@andreialecu
Jan 30 2015 21:21
Try deleting node_modules before running npm install, might be an issue with the git dependency not updating properly
Nicolas Ritouet
@NicolasRitouet
Jan 30 2015 21:22
I do that automatically
ok, now it works
probably a zombie process
Andrei Alecu
@andreialecu
Jan 30 2015 21:23
Subsequent runs are ok?
Nicolas Ritouet
@NicolasRitouet
Jan 30 2015 21:23
apparently yes
one thing was weird: it was not running the integration test and was silently skipping them (and the tests were passing)
ok, I get it
so, dpd is still running after the tests in zombie mode
when I try to run another dpd instance, I get this:
ERROR: port 2403 is already in use
Trying again on port 2404...
ERROR: port 2404 is already in use
Trying again on port 2405...
ERROR: port 2405 is already in use
Trying again on port 2406...
ERROR: port 2406 is already in use
Trying again on port 2407...
and again and again ...
for each new test, it uses a new port
Andrei Alecu
@andreialecu
Jan 30 2015 21:26
On Windows when the main process exits all forks exit too
Looks like it's different on Mac
I didn't explicitly exit it because of this
What is your node --version?
Nicolas Ritouet
@NicolasRitouet
Jan 30 2015 21:28
ok, and I have the error I mentioned before: when dpd fails to start, the test are skipped and the batch of test pass (even though we haven’t ran the integration tests)
v0.10.35
Andrei Alecu
@andreialecu
Jan 30 2015 21:36
I just pushed a change from phone, try now, I think it should fix both issues
Nicolas Ritouet
@NicolasRitouet
Jan 30 2015 21:38
apparently, there’s still a mongodb process that hangs
hold on
maybe not
no, there is :(
Andrei Alecu
@andreialecu
Jan 30 2015 21:47
What about it?
Nicolas Ritouet
@NicolasRitouet
Jan 30 2015 21:48
dpd fork a new process for mongodb
and when we kill dpd, we don’t kill his childprocess mongodb
which means that we also have zombie process of mongod
Andrei Alecu
@andreialecu
Jan 30 2015 21:48
There is a handler at the bottom
For exit
Nicolas Ritouet
@NicolasRitouet
Jan 30 2015 21:49
but apparently, it’s not taken in account
Andrei Alecu
@andreialecu
Jan 30 2015 21:49
Which should kill mongo
Seems to work on Windows
first time, clean env
second time, after a npm test
third time, after a second npm test
Andrei Alecu
@andreialecu
Jan 30 2015 22:03
That should have worked
If the normal kill didn't
Nicolas Ritouet
@NicolasRitouet
Jan 30 2015 22:03
I’m trying this
pid = fs.readFileSync('./.dpd/pids/mongod'); if(pid) { console.log('pid %s', pid); process.kill(pid); } else { console.log('no pid found'); }
Andrei Alecu
@andreialecu
Jan 30 2015 22:06
The second call should kill it if it was still running based on that code that was already there
Nicolas Ritouet
@NicolasRitouet
Jan 30 2015 22:07
this works
Andrei Alecu
@andreialecu
Jan 30 2015 22:07
Maybe kill isn't killing mongo?
Nicolas Ritouet
@NicolasRitouet
Jan 30 2015 22:08
indeed, that’s the issue
we have to do it manually
Andrei Alecu
@andreialecu
Jan 30 2015 22:08
Try SIGKILL
Nicolas Ritouet
@NicolasRitouet
Jan 30 2015 22:08
I tried all of them
none works
Andrei Alecu
@andreialecu
Jan 30 2015 22:08
Instead of sigterm which is default
Nicolas Ritouet
@NicolasRitouet
Jan 30 2015 22:08
SIGTERM, GITHUP, etc...
unfortunately, I don’t have much time, I have to leave now
Can I let you update the PR with this
and I’ll ask Eric to rebase it
Andrei Alecu
@andreialecu
Jan 30 2015 22:10
Should it be the same PR?
Nicolas Ritouet
@NicolasRitouet
Jan 30 2015 22:11
yes
in run test, in the kill method
you add a comment: // kill mongo child process
and the code I showed you
Andrei Alecu
@andreialecu
Jan 30 2015 22:11
That isn't responsible for killing mongo
Nicolas Ritouet
@NicolasRitouet
Jan 30 2015 22:11
it is
Andrei Alecu
@andreialecu
Jan 30 2015 22:12
The problem is in mongo js
Which wasn't the scope of this PR
If it isn't killing it
No?
runtests doesn't touch the mongo process
Nicolas Ritouet
@NicolasRitouet
Jan 30 2015 22:14
it start dpd and dpd itself starts a mongo process
but since it’s a child of a child, we don’t have direct control on it
only through the pid
Andrei Alecu
@andreialecu
Jan 30 2015 22:16
Hard to type on phone but that isn't relevant because the code saves mongo's pid and a subsequent run will kill it
If it couldn't kill it by exit event like it should have
Nicolas Ritouet
@NicolasRitouet
Jan 30 2015 22:17
run of npm test? because the more npm test I run, the more zombie mongo process I have
Andrei Alecu
@andreialecu
Jan 30 2015 22:17
If process.kill doesn't kill it then something is wrong there
Nicolas Ritouet
@NicolasRitouet
Jan 30 2015 22:18
apparently, dpd CLI somehow can kill it properly, since I don’t see the process running
Andrei Alecu
@andreialecu
Jan 30 2015 22:18
If you run dpd and kill it manually does that leave zombies?
Nicolas Ritouet
@NicolasRitouet
Jan 30 2015 22:18
manually, you mean with a ctrl+c ? then no, I don’t have zombies
but if I kill dpd start with a kill -9, then the mongo process is zombiing
I really have to leave
I let you find a solution, I’ll tell Eric to review this and to rebase if it’s ok
for me, if you add the code I showed you in the kill method inside runtest.js, I’m good with that
Andrei Alecu
@andreialecu
Jan 30 2015 22:22
I'll look at it
Nicolas Ritouet
@NicolasRitouet
Jan 30 2015 22:23
ok
bye Andrei
Andrei Alecu
@andreialecu
Jan 30 2015 22:23
Bye, have fun in your vacation
Nicolas Ritouet
@NicolasRitouet
Jan 30 2015 22:23
thx :)