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

support YAML for app declaration #578

Closed
rlidwka opened this issue Jul 16, 2014 · 18 comments
Closed

support YAML for app declaration #578

rlidwka opened this issue Jul 16, 2014 · 18 comments

Comments

@rlidwka
Copy link
Collaborator

rlidwka commented Jul 16, 2014

I'm looking at this, and just realizing that is not a meteor issue. It's pm2 issue. JSON just doesn't work quite right when used for config files.

If we add yaml support, @dandv 's example could be rewritten like this:

name: frontend
script: main.js

env:
  NODE_ENV: production
  METEOR_SETTINGS: |
    {
      "Kadira": {
         "appId": "...",
         "appSecret": "..."
      },
      "public": {
        "analytics_api_key": "xxx",
        ...
      }
    }

... solving all issues with escaping quotes.

I don't even mention that in production environments those files need to be commented out (which json doesn't support), or a handy feature of yaml that allows to define constants and reuse it anywhere in the same file (say, define "max_restarts" once and use that value for every app).

This would also remove the need for single quotes in "args" (and therefore eval(), see #568, which one could see as a possible security issue), replacing that with double quotes and a regular JSON.parse().

@dandv
Copy link
Contributor

dandv commented Jul 16, 2014

JSON is surprisingly bad for config files, yet it continues to be used.

@soyuka
Copy link
Collaborator

soyuka commented Jul 17, 2014

js is for me a better alternative than yaml but that's an endless debate.

I don't understand why this won't work:

"env": {
    "NODE_ENV": "production",
    "METEOR_SETTINGS": {
      "Kadira": {
         "appId": "...",
         "appSecret": "..."
      },
      "public": {
        "analytics_api_key": "xxx",
        ...
      }
    }

@rlidwka
Copy link
Collaborator Author

rlidwka commented Jul 17, 2014

js is for me a better alternative than yaml

Since json is a subset of yaml, you can use json even inside yaml config. So it isn't really about an alternative, it is about opening a possibility for better config files for other people.

An alternative would be to use json5, it's basically a javascript syntax with json-only data. But what I like in yaml is something like this:

[{
  "name"      : "api",
  "script"    : "./api.js",
  "instances" : &instances 4,
  "env": &my_env {
    "NODE_ENV": "production",
    "AWESOME_SERVICE_API_TOKEN": "xxx",
  }
},{
  "name"      : "frontend",
  "script"    : "./frontend.js",
  "instances" : *instances,
  "max_restarts": &restarts 15,
  "env"       : *my_env,
},{
  "name"      : "backend",
  "script"    : "./backend.js",
  "max_restarts": *restarts,
  "instances" : *instances,
  "env"       : *my_env,
}]

This is a valid yaml document. Do you see what I've done here?

If you have multiple applications in your services.json file, a lot of information would be duplicated. Say, instances or max_restarts could be different from the default, but the same for some of your apps.

With yaml one could create a constant somewhere in the file, and reuse it later in the same file. If we need to change a constant (say, API tokens are changed frequently), in JSON you would have to replace it in dozen places, but with this configuration you can do it just once.

YAML ain't ideal, but in the case of pm2 it seems like the best fit.

@soyuka soyuka added need pr and removed need pr labels Jul 29, 2014
@soyuka
Copy link
Collaborator

soyuka commented Aug 14, 2014

#248

Also we should require() instead of readFileSync here: https://github.com/Unitech/pm2/blob/master/lib/CLI.js#L230

So we might use pure js.

@rlidwka
Copy link
Collaborator Author

rlidwka commented Aug 15, 2014

Pure js is a script format, not a data format, current "configuration" syntax won't work so nicely there.

If you want to use js, maybe add high-level syntax. Instead of:

{
  "name"      : "api",
  "script"    : "./examples/child.js",
  "instances" : "4",
  "error_file" : "./examples/child-err.log",
  "out_file" : "./examples/child-out.log"
}

Write something like:

require('pm2').create_app('api')
  .script('./examples/child.js')
  .instances(4)
  .error_file('./examples/child-err.log')
  .out_file('./examples/child-out.log')

Maybe it's already possible somehow.

@soyuka
Copy link
Collaborator

soyuka commented Aug 16, 2014

Nope it's only that you can require json files and so, you would be able to do a configuration like this:

module.exports = {
    scripts: 'test.js',
    instances: 4
}

@Unitech
Copy link
Owner

Unitech commented Nov 10, 2014

JSON5 support has been added, try the #development branch

@Unitech Unitech closed this as completed Nov 10, 2014
@naggie
Copy link

naggie commented Mar 25, 2015

Yaml support would still be nice. May I add it?

@jshkurti
Copy link
Contributor

Sure :)

@Unitech
Copy link
Owner

Unitech commented Mar 25, 2015

Yes would be nice, @naggie do you know any lightweight yaml library for node? (without too many dependencies)

@soyuka
Copy link
Collaborator

soyuka commented Mar 25, 2015

@Unitech @naggie I'm using https://www.npmjs.com/package/yamljs

@michaelBenin
Copy link

I would like to +1000 this. I like having comments in my configuration file.

@naggie
Copy link

naggie commented Mar 25, 2015

Great, I'll do it this evening. @michaelBenin BTW I think you can put in comments anyway with the current JSON5 support: http://json5.org/. Nonetheless, YAML files are nicer for humans to read/write; also allowing anchors & references.

@naggie
Copy link

naggie commented Mar 31, 2015

I have 2 options:

  1. Drop JSON5 support in favour of YAML and risk the PR not being accepted. Bear in mind that JSON is a subset of YAML so we can still use JSON files; even commented JSON files
  2. Change parser by file extension. This might not be straightforward as files are loaded into strings and passed around in CLI.js sometimes.

I would prefer option (1)

N.B. I noticed a few instances that might not work with the JSON5 parser -- a filename is given instead of the string

@soyuka
Copy link
Collaborator

soyuka commented Mar 31, 2015

You can also test the first character of the string that's not a space or a
new line. If it's { or [ it's json if not it's yaml. Solution one, will
indeed bring a compatibility issue and therefore won't be merged...
If a file is sent as an argument testing the file extension seem fair to me.
Le mar. 31 mars 2015 à 10:20, Callan Bryant notifications@github.com a
écrit :

I have 2 options:

  1. Drop JSON5 support in favour of YAML and risk the PR not being
    accepted. Bear in mind that JSON is a subset of YAML so we can still use
    JSON files; even commented JSON files
  2. Change parser by file extension. This might not be straightforward
    as files are loaded into strings and passed around in CLI.js sometimes.

I would prefer option (1)

N.B. I noticed a few instances that might not work with the JSON5 parser
-- a filename is given instead of the string


Reply to this email directly or view it on GitHub
#578 (comment).

@felixfbecker
Copy link

I also think PM2 should accept multiple file types for app declarations. _It currently supports embedding JS inside JSON!_ If you write Javascript inside a "JavaScript Object Notation" file you might as well just put in a JS file with module.exports =. PM2 should accept .yaml, .json and .js.
JSON really is bad without official support for comments and all the quoting going on.

@Unitech
Copy link
Owner

Unitech commented Mar 25, 2016

Feature landed in development branch, thanks to the lightweight YAML parsing module https://www.npmjs.com/package/yamljs

@Unitech Unitech closed this as completed Mar 25, 2016
@Unitech
Copy link
Owner

Unitech commented Apr 3, 2016

Update available (PM2@1.1.1 - latest stable):

$ npm install pm2@latest -g
$ pm2 update

Please re-open this issue if you notice any problem,

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

No branches or pull requests

8 participants