Skip to content
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

Added Procfile to repository root #402

Closed
wants to merge 2 commits into from

Conversation

samis
Copy link
Contributor

@samis samis commented Apr 18, 2015

This adds a simple Procfile to the root of the repository, allowing the tool 'foreman' to be used to stop/start the bot. Aside from providing prettier console output, it will make it easier to manage the bot if it ever becomes multi-process (e.g a Redis server or a component written in another language)

@botwillacceptanything
Copy link
Owner

☑️ Voting procedure reminder:

To cast a vote, post a comment containing 👍:+1:, or 👎:-1:.
Remember, you must ⭐star this repo for your vote to count.

All comments within this discussion are searched for votes, regardless of the time of posting.
You can cast as many votes as you want, but only the last one will be counted.
(You may consider editing your comment instead of adding a new one.)
Comments containing both 👍up- and 👎down-votes are disregarded.
Comments containing monkeys are disregarded. 🐒 🐵 🙈 🙉 🙊
PR authors automatically count as a 👍 vote.

A decision will be made after this PR has been open for 15 minutes (plus/minus 10 percent, to avoid people timing their votes), and at least 8 votes have been made.
A supermajority of 65% is required for the vote to pass.

NOTE: the PR will be closed if any new commits are added after:
6394b92

@coveralls
Copy link

Coverage Status

Coverage remained the same at 27.72% when pulling 6394b92 on samis:foreman-procfile into 24fccf0 on botwillacceptanything:master.

@shalecraig
Copy link
Contributor

👍

@SpenserJ
Copy link
Contributor

Is there a node process manager that we can use instead of depending on Ruby?

@samis
Copy link
Contributor Author

samis commented Apr 18, 2015

Just found one. https://github.com/strongloop/node-foreman.
I'll test it now.

@samis
Copy link
Contributor Author

samis commented Apr 18, 2015

....damm. 'If your processes exit, Node Foreman will assume an error has ocurred and shut your application down.'

@samis
Copy link
Contributor Author

samis commented Apr 18, 2015

Just tested it. node-foreman does indeed start the bot successfully. The problem of the entire app terminating when a process exits will need to be dealt with however.

@RyanCopley
Copy link

👍

@SpenserJ
Copy link
Contributor

Does it only assume it was bad if process.exit() returned a non-zero error code? We can just return 0 whenever we want to restart

@samis
Copy link
Contributor Author

samis commented Apr 18, 2015

Hmm. Let me see if I can look at the code and see.

@samis
Copy link
Contributor Author

samis commented Apr 18, 2015

@SpenserJ
Copy link
Contributor

What if we moved some logic into launcher.js, and keep that process running. When main wants to restart, it pings the launcher, the launcher gracefully closes the process (so that the DB disconnects and such), and then restarts the process. If launcher.js receives an exit command, it can trigger the same graceful shutdown on main.js

@samis
Copy link
Contributor Author

samis commented Apr 18, 2015

so essentially, make launcher.js the supervisor of main.js and launch that from the Procfile instead?

@SpenserJ
Copy link
Contributor

Yes, but I don't know whether we even need the Procfile or Foreman at that point. launcher.js would provide stop/start functionality at that point. I don't know what else Foreman adds, but it could be useful for launching other processes, like you mentioned.

@samis
Copy link
Contributor Author

samis commented Apr 18, 2015

You have a fair point. However, were we to add something requiring an additional (I'm going to use Redis as an example as I plan to add Redis support shortly) it'd be easier to add an additional line in the Procfile rather than ensure the service is started manually or by the system's init daemon. That would apply mostly to development but also has effects on production.

@SpenserJ
Copy link
Contributor

Sounds like a good setup then. launcher.js controls start/stop functionality for main.js, and Node Foreman handles the procfile for launcher.js and redis

@samis
Copy link
Contributor Author

samis commented Apr 18, 2015

Should I close and reroll then?

@SpenserJ
Copy link
Contributor

That'd be great. I'd say our new solution should hit all the points.

@dbpokorny
Copy link
Contributor

Is the plan

(1) to have a /foreman http endpoint that lists foreman processes the bot has started, state of the processes (for example "show processlist" in mysql)

(2) allow the bot to start and stop services through irc? "anythingbot, start redis", "anythingbot stop mysql"

(3) start, restart, and stop services by PR

@samis
Copy link
Contributor Author

samis commented Apr 18, 2015

@dbpokorny I hadn't thought of that. That would obviously require the reroll of this to be merged though. Some of those might be difficult though, depending on which implementation of foreman is used (I think they might have some differences)

@dbpokorny
Copy link
Contributor

@samis I don't see what choice there is to make? https://www.npmjs.com/package/foreman

@samis
Copy link
Contributor Author

samis commented Apr 18, 2015

@dbpokorny Good choice. I'll go see which of your ideas might be difficult to use with that package.

@samis
Copy link
Contributor Author

samis commented Apr 18, 2015

The 'restart' bit will require either using a fork, or for a pull request to get merged. also, that will either need a global install (requires root) or modifying the PATH to include the path to node-foreman. Additionally, listing processes is not inbuilt into node-foreman but could likely be achieved by parsing the output of node-foreman.
Does that make sense?

@dbpokorny
Copy link
Contributor

@samis I'm not familiar with this, but I trust your judgement (like I have a choice lol :-)

@samis
Copy link
Contributor Author

samis commented Apr 18, 2015

Fair enough. Closing now to reroll.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants