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

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

Open
fdddf opened this issue Apr 26, 2023 · 10 comments
Open

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

fdddf opened this issue Apr 26, 2023 · 10 comments
Labels
bounty:$10 Bounty applies for fixing this issue (Parse Bounty Program) type:bug Impaired feature or lacking behavior that is likely assumed

Comments

@fdddf
Copy link

fdddf commented Apr 26, 2023

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 #2113 (comment)

@parse-github-assistant
Copy link

Thanks for opening this issue!

@mtrezza mtrezza added the type:bug Impaired feature or lacking behavior that is likely assumed label Apr 26, 2023
@mtrezza mtrezza added the bounty:$10 Bounty applies for fixing this issue (Parse Bounty Program) label Jun 5, 2023
@patelmilanun
Copy link
Member

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

@mtrezza
Copy link
Member

mtrezza commented Jun 18, 2023

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
Copy link
Member

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

@mtrezza
Copy link
Member

mtrezza commented Jun 18, 2023

Great! Before you make extensive changes about it in the README, you may want to take a look at #2465.

@patelmilanun
Copy link
Member

patelmilanun commented Jun 18, 2023

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
Copy link
Member

patelmilanun commented Jun 18, 2023

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
Copy link
Member

mtrezza commented Jun 18, 2023

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
Copy link
Member

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
Copy link
Member

mtrezza commented Jun 18, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bounty:$10 Bounty applies for fixing this issue (Parse Bounty Program) type:bug Impaired feature or lacking behavior that is likely assumed
Projects
None yet
Development

No branches or pull requests

3 participants