-
Notifications
You must be signed in to change notification settings - Fork 3.9k
[TODO] Simpler configuration #956
Comments
Hey! Great points, and you are 100% right. Let me quickly explain how we got to the current mess:
TLDR:
So what would you think is ideal? I don't think I want to fully remove single config files just yet (and use the TOML config files for configuring CLI gekko). |
You are right about the backend, but the frontend needs to know where the backend is. That's the whole reason for that file: so the frontend can figure out how to contact the backend. |
Two problems with this:
|
Thanks for your explanation! As I understand right now the config.js contains most of the configuration needed by both the backend and the UI.
Now, let's say I download the application and I know all configuration are in a sample-config.js
I think in this scenario both the simple usability (as you stated user requested) and simple configuration are taken care of. There's nothing to configure by default A couple of points on the UI design:
I tried to explain as best as I could, hope this helps ;) |
I think the config should be in one place but I don't like the comments about the api server port etc. That should all be in the main config, but it should be easy to change the port in the config. TBH, the UI is way too restrictive to be worth using imo and is now forcing me to manage settings in the config file and a Tomlinson, which is pointless. Indeed the overhead for the front end to function is not worth it. I tried it again earlier and it's still calling the api twice with large datasets, it's graphing of indicators isn't great and I just don't see how you can learn from it. Being noob friendly seems fine so long as it's actually possible for people to install, use the native indicators and make money. But is that actually the case? |
Quick notes before I go into it:
No, Gekko will crash with an error that you didn't specify a config. Editing the sample-config is a bad idea since it makes updating Gekko really hard (merge conflicts inside sample-config).
This only works if the host is specified somewhere, since most people runnig Gekko in the cloud don't connect the UI directly to the gekko server but route traffic through nginx. And if it is specified somewhere we can also give it to the frontend :) (calling Another reason for the UIConfig is so that if Gekko runs on the same machine as your browser Gekko will open the browser for you, pointing to the UI (as configured in the UIConfig).
Great! As long as it works in node6 without transpiling.So if I get you right, your main point is:
Where the first part also houses stuff like DB settings and such. These two parts are used for both CLI and UI (where applicable). This sounds perfect, though I am worried about the following use cas:
I won't go into it too deep, but I want to abstract the config file from the user (when using the UI), but definitely not from the rest API: this should be a simple API wrapper around starting and managing gekkos. And the configs provide a lot of flexibility, why move away from this? Maybe it is better to limit this issue to user facing configuration. API communication between the rest server and UI is a bit out of scope for this issue imo. |
Do you have a proposal in mind for a (dummy / pseudo) file structure, and how it would work from the user when running over the CLI (what does the user need to edit when)? |
An easier option would be:
Wouldn't this solve a lot of confusion? |
I agree with this! I think the sample-config.js can stay on the root a UI user should just "npm start" the project if thats the default in the docs
It would be correct to merge baseConfig with the config.js (alias sample-config.js). Have UIConfig.js only to define UI concerning settings like API configs. Moving it to the root is a big step forward 👍
In a UI startup, if the user has not created the config.js, the server could copy config-sample.js and use it as default config, the user MUST not edit the config-sample.js
I don't think there's need for the server to crash, reading the config is a script and errors could be handled to copy and use the copy of the sample-config.js instead (probably not for the CLI as you said)
In fact, the config.js (alias sample-config.js) remains almost the same as it is right now.
http://es6-features.org/#StringInterpolation
Recap of what just said:
Do you think this solves the different scenarios? |
I am not sure how StringInterpolation can convert JSON into TOML dynamically? If there is a lib we can def use it though :)
Yes this is always needed, since if users update the config.js we need to make sure these changes are applied to the UI. In that case we need some heavy comments in all the toml files saying that you should NEVER edit these files manually, since they will always be overwritten on start. But in that case why write the TOML files to disk at all? Just render them on the fly in the API and feed into the frontend?
Yes great! I would like to keep these configs apart, since it highlights that a |
For what I've seen TOML are just some
Actually I don't know how TOML files are used, I just assumed that you needed them to be saved for some purpose, maybe to keep configs upon a restart. I don' know :)
Keeping in mind that if a user wants to connect a remote UI, he will need that socket open. That could be still enable/disable setting in the config.js (nearby the port config in which the CLI would open the socket for remote or local UI connections) |
A little bit more, but not too much (nested props, arrays). You are right it should not be too hard.
Ah, they are only given to the frontend so that it can render config blocks (a proper solution would be a dynamically generated form based on the JSON - this means we can ditch TOML all together). Since they are only used for that we don't even need to write them to disk. So what I propose (95% the same as your proposal): (easiest - stick to TOML for UI config)
(more work - ditch TOML)
Would that be acceptable? Main thing we would need (for
To be clear:
So only in the second situation a config with any host/port is used (the UIConfig) - the first option ignores this and does not open ports. |
The question might be are this extented features actually in use or they could be simplified?
Seems a very good solution
I would open another issue for this
Yeah, this kinks of explains better how things are going under the hood |
Nested is used a lot, basically all nested objects (more than 1 layer deep) are nested inside the TOML (example - everything under simulationBalance is nested inside the key Okay great! We have our work cut out for us. Would you be willing to work on this? Else I would put it on the end of my TODO. |
I see if I can find some time to get to work on something in the weekend, maybe starting to move and fix the config file paths. I'm very new to gekko so about testing the whole application feature might be difficult to me, but I think you could work that out |
Awesome thanks! That lib looks great, we could use that straight away (if it works). I think the easiest start would be creating a After that you can fully remove the |
Is there any reason why the UI has to use TOML? Could it just be JSON instead? JSON syntax is going to be more familiar for anyone using it. I'd never even heard of TOML before (and still really unfamiliar how to deal with nesting, arrays, etc). I'd be glad to help work on converting it over because I'm running into an issue where running my strategy in the CLI now requires a separate TOML config file now to run it in the UI. Mainly I want to see the buy and sells plotted out and it's inconvenient they're separate (unless they aren't and I'm missing something). |
@Lbatson I missed your reply apologies:
The main goal of the UI is to allow for non technical users to use Gekko. I know we are not quite there (you need to manually install gekko's dependencies via NPM, start gekko via CLI), but I want to move towards that goal. People who are very familiar with JSON (it's actually a hard syntax for non tech: forget one
In the direction I am going I want to separate users from producers, only the latter need to worry about how to map a TOML file to some struct. The first can just "fill in the TOML". 2 possible solutions to what you describe:
|
I'm afraid this won't be coming anytime soon :( In the new UI will tackle server configuration (host, ports, auth, etc) through a wizard, not via a config file anymore. Also will probably factor out TOML files. |
It took some time to trying to understand the project configurations, which is poorly documented (I understand this takes time)
I also had some troubles configuring the app in a "production" server, due to the many configurations all around the project
A few improvements could be implemented:
I see many duplicated configurations inside the project (toml files, config-sample.js => config.js, UIconfig.js, baseConfig.js). I don't really understand the reason for this, but I think it would be a lot simpler for both the cli and the backend in general to have a unique centralized configuration in the config.js (with the config-sample.js committed) and to remove all the other files.
The config.js is where the user would look into immediatly and understand where to put his personalizations.
baseConfig.js contains mostly duplicated configs from config.js (or viceversa), which could easily allow to merge in a unique config.js
Also, in my opinion it, UIconfig.js should not need to contain backend configs like "API url", usually the backend who serves the UI is it's own API backend so relative urls would be enough. In case the backed is actually a different URL it could be the backend itself to serve the UI that information (moving again the configurationt to the backend config.js)
At this point the UIconfig.js is empty :)
One single config.js file, it's simpler to configure but also simpler to document (resolving the first stated issue)
It's a bit of info, but I hope it helps!
The text was updated successfully, but these errors were encountered: