Skip to content
This repository was archived by the owner on Sep 21, 2022. It is now read-only.

Executable config#385

Merged
j0tunn merged 2 commits intomasterfrom
feature/executable.conf
Mar 14, 2016
Merged

Executable config#385
j0tunn merged 2 commits intomasterfrom
feature/executable.conf

Conversation

@j0tunn
Copy link
Copy Markdown
Contributor

@j0tunn j0tunn commented Mar 9, 2016

No description provided.

- ability to read .js/.json files
- default config file moved to Config module
@levonet levonet added the review label Mar 9, 2016
@SevInf
Copy link
Copy Markdown
Contributor

SevInf commented Mar 9, 2016

What problems does the executable config solves which current config and plugins can not solve? YAML config is nice and readable and I can get what is going on with just glance on it which is not the case, for example, enb and Karma config.
Config already supports overriding options, plugins can be used to launch something before or after the launch or alter config before execution. So, what does it solves exactly?

@j0tunn j0tunn force-pushed the feature/executable.conf branch from 7968d56 to df71596 Compare March 9, 2016 10:42
@j0tunn
Copy link
Copy Markdown
Contributor Author

j0tunn commented Mar 9, 2016

@SevInf It allows the communication between plugins. For example we can pass function as an option to some plugin.
Anyway this PR adds ABILITY to use executable config. One can still use .yml

@SevInf
Copy link
Copy Markdown
Contributor

SevInf commented Mar 9, 2016

@j0tunn I assume it is for gemini-testing/gemini-tunnel#9.
What kind of complicated logic for picking a free port requires a function and why this function can't be a part of a plugin?

@j0tunn
Copy link
Copy Markdown
Contributor Author

j0tunn commented Mar 9, 2016

The problem is we should start server first (thats a not a vanilla http server). And we have a plugin for that. Both plugins should know about that free port.

@SevInf
Copy link
Copy Markdown
Contributor

SevInf commented Mar 9, 2016

Why the plugin thats start the server can't just set the port for a tunnel once it started the server?

@j0tunn
Copy link
Copy Markdown
Contributor Author

j0tunn commented Mar 9, 2016

The order of plugin execution is not guaranteed. And free port allocation is an async operation

@SevInf
Copy link
Copy Markdown
Contributor

SevInf commented Mar 9, 2016

Async is not a problem, startRunner event uses emitAndWait. Regarding the order:

  1. Plugins are executed in order of keys of plugins config object, which is guaranteed in ES2015 by standard and de-facto guaranteed in ES5 by v8.
  2. Making the order of plugins defined will be generally a good idea.

@j0tunn
Copy link
Copy Markdown
Contributor Author

j0tunn commented Mar 9, 2016

Async in fact is a problem, because emitAndWait doesn't help us: second plugin should start only when first has finished its work.
Also it would be not very nice to have plugin that somehow affects other plugin (silently)

@j0tunn
Copy link
Copy Markdown
Contributor Author

j0tunn commented Mar 9, 2016

Why are you against the .js configs?

@SevInf
Copy link
Copy Markdown
Contributor

SevInf commented Mar 9, 2016

Config file should be declarative and easy to read. Any JS config I've ever seen is a horrible mess that requires you to gaze into it for a long time to get what is going on there (enb, grunt, karma, webpack, list continues).
A lot of effort was put into making sure that existing config is powerful enough without needing to be executable. The stuff like per-browser overrides, env variables overrides or plugins probably woudn't be there if we went with js configs from the beginning, but it keeps the config easy to read without having mentally execute JS.
Yes, YML configs are not going away, but existence of JS config encourages bad practices.

@j0tunn
Copy link
Copy Markdown
Contributor Author

j0tunn commented Mar 9, 2016

A lot of effort was put into making sure that existing config is powerful enough without needing to be executable. The stuff like per-browser overrides, env variables overrides or plugins probably woudn't be there if we went with js configs from the beginning, but it keeps the config easy to read without having mentally execute JS.

All this stuff will help to make config easy and readable

@SevInf
Copy link
Copy Markdown
Contributor

SevInf commented Mar 9, 2016

No, it will not, because why would you go through publishing a plugin when you can just write all the stuff you need inside a config? Why would you configure environment on your CI if just can do if (isDev) computeConfigA() else computeConfigB() Nobody will care about config transforming to program until it becomes another enb/make.js.

@j0tunn
Copy link
Copy Markdown
Contributor Author

j0tunn commented Mar 9, 2016

why would you go through publishing a plugin when you can just write all the stuff you need inside a config?

to localise specific functionality. Yes, I'll do it

Why would you configure environment on your CI if just can do if (isDev) computeConfigA() else computeConfigB()

Don't you think that it would be convenient to make such configuration in gemini config file instead of some top level script, that will generate .gemini.yml and than launch gemini with that config? Just because gemini can't read executable script. Thats a real case, we already have such project!

Nobody will care about config transforming to program until it becomes another enb/make.js

Thats totally up to the users. If they want to have simple config - they will have simple config, gemini provides ability for that. If they don't - they will have totally messy crap around gemini anyway.

Such restriction just moves complexity from gemini config to some additional stuff.

@SevInf
Copy link
Copy Markdown
Contributor

SevInf commented Mar 9, 2016

Don't you think that it would be convenient to make such configuration in gemini config file instead of some top level script, that will generate .gemini.yml and than launch gemini with that config

I don't think either approach is good. In the end, if the plugins are so dependent on each other why are they separate? All need for for executable config will go away if you just merge them into one.

Before you say "We want to keep public tunnel plugin": plugin is just a function and nothing prevents your private plugin from depending on it and doing require('gemini-tunnel')(gemini, {port: 123456}) when you know the port.

@j0tunn
Copy link
Copy Markdown
Contributor Author

j0tunn commented Mar 9, 2016

We want to use tunnel plugin with or without server plugin. One project - only tunnel plugin, other project - tunnel with server. They are about different stuff, they can work separately. They will work separately. Why should we join them? Just to keep our config simple? Just to keep somebody's config simple?

Your point is "Do any spikes you need, but don't make config executable". But we want a simple straightforward way to do some configuration stuff. Config file is the best place for that stuff.

Again, this PR adds ABILITY to do more complicated things in config. Still anyone can use simple .yml configs.

@j0tunn
Copy link
Copy Markdown
Contributor Author

j0tunn commented Mar 10, 2016

@SwinX @sipayRT @arikon take a look, please


/**
* @param {Object|String} configData data of the config or path to file
* @param {Object|String} config config or path to config file
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

config config

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ну да, сначала имя параметра - config, а затем описание - config or path to config file

@j0tunn j0tunn force-pushed the feature/executable.conf branch from df71596 to d00517a Compare March 14, 2016 10:43
j0tunn added a commit that referenced this pull request Mar 14, 2016
@j0tunn j0tunn merged commit 09735bd into master Mar 14, 2016
@j0tunn j0tunn deleted the feature/executable.conf branch March 14, 2016 10:53
@levonet levonet removed the review label Mar 14, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants