parse-community / parse-dashboard

A dashboard for managing Parse Server
https://parseplatform.org
Other
3.74k stars 1.38k forks source link

port in parse-dashboard config.json doesn't work #2429

Open fdddf opened 1 year ago

fdddf commented 1 year ago

Enviroment: parse-dashboard 5.0.0 / parse-server 6.0.0 The port in --config dashboard.json doesn't work, the only way to work is give a particular port with --port:

parse-dashboard --config dashboard.json  --port 8080

dashboard.json

{
        "apps": [{
                "serverURL": "https://server.com/parse",
                "appId": "parseserver",
                "masterKey": "keykey",
                "allowInsecureHTTP": "true",
                "port": 8080,
                "appName": "BaaS Adhoc"
        }]
}

I am not sure port should be place into the root or apps in dashboard.json, but it doesn't works as I have tried both.

Originally posted by @fdddf in https://github.com/parse-community/parse-dashboard/issues/2113#issuecomment-1521438890

parse-github-assistant[bot] commented 1 year ago

Thanks for opening this issue!

patelmilanun commented 1 year ago

@mtrezza do we even have port as config parameter inside that config file?

mtrezza commented 1 year ago

It seems that --port is a CLI parameter. If it isn't supported in the config file, this would be feature request to add it. This is also a good indication that we should add a config options table to the README.

patelmilanun commented 1 year ago

ok i'm on the way to adding port as config file parameter.

mtrezza commented 1 year ago

Great! Before you make extensive changes about it in the README, you may want to take a look at https://github.com/parse-community/parse-dashboard/issues/2465.

patelmilanun commented 1 year ago

I'm thinking of refactoring of the file. So I want to do something like if same parameter is passed from file as well as command line arg, the command line arg should take precedence instead of throwing error like "it should be either file or CLI oprion." And also add same options to .env as well.

Basically adding same options to CLI, env, and config file. And precedence order from low to high will be CLI < config file < env < default hardcoded value.

Example const host = options.host || configFile.host || process.env.HOST || '0.0.0.0';

What are ur thoughts @mtrezza

patelmilanun commented 1 year ago

Here is my algorithm to do the same

steps to get values (every step is optional in a way that we don't required the input to that step like file or env or CLI options etc, as we are not concerned with the steps but the values)

  1. Extract values from options (CLI options) variable
  2. Parse config file if invalid throw "Your config file contains invalid JSON. Exiting." but continue
  3. Extract values from env
  4. Attach a default value which is hardcoded to fallback on
  5. Check if all required values are present then run server else throw the missing parameters which all are required
mtrezza commented 1 year ago

It sounds good, just a note... if we change the precedence order it would be a breaking change; if there is a good reason (i.e. improvement) we can have a breaking change, otherwise we try to avoid it. If the dashboard currently just crashes / throws an error and stops init when a param is set via CLI and config and env var, then I'd not consider it a breaking change.

Maybe leave this open for discussion a few days, in case there is any additional input.

patelmilanun commented 1 year ago

I mean we are just making it clear about preceding order. It is almost exactly as it is right now. Still, I'll run through edge case myself and report here.

mtrezza commented 1 year ago

Sure, it's probably unlikely that someone has the same param set in config as in CLI or env var; but if someone has it may break their deployment. So it would be interesting to know how it currently behaves in these cases.