search

conwetlab / ngsi-proxy

Proxy allowing receiving notifications from Orion Context Broker on browser environments
GNU Affero General Public License v3.0
8 stars 9 forks source link

Feature/docker #9

Closed jason-fox closed 5 years ago

jason-fox commented 5 years ago

This PR Updates the NGI Proxy Docker build to do the following

coveralls commented 5 years ago

Pull Request Test Coverage Report for Build 10

Files with Coverage Reduction New Missed Lines %
logic.js 17 82.18%
<!-- Total: 17 -->
Totals Coverage Status
Change from base Build 6: -6.1%
Covered Lines: 214
Relevant Lines: 250
💛 - Coveralls
aarranz commented 5 years ago

PR looks good to me, except pm2 part. Why to use a process manager taking into account that we are using docker? docker-compose/docker swarm has support for restarting dead or unhealthy containers. Moreover, docker-compose/swarm allows you to configure restart policy, how to detect if the container is unhealthy, and so on. By using pm2 in the proposed way (not providing a way for configuring pm2), we are fixing the behaviour removing flexibility and not adding any feature.

In fact, some of the bugs FICODES has fixed on the IoTAgents are related to having IoTAgents entering in zombie mode (process running but not working as expected) problems that were not fixed by adding pm2. So, in my honest opinion, pm2 is not providing any advantage on the IoTAgents images.

jason-fox commented 5 years ago

@aarranz - I'll remove pm2 for now, but ideally I'd like to get a common way forward for this. I only added it in the first place for consistency with the IoT Agents.

Whether pm2 is a good thing, a bad thing or neither, I'd leave to the discussion thread

jason-fox commented 5 years ago

PR looks good to me, except pm2 part.

pm2 removed (at least for now) . Fixed: 0e5b9e5, 62a5b22

aarranz commented 5 years ago

I don't know why coverage decreased, as you are not changing the code nor the tests...