if config.example.js does not exist, a service can crash

[kirrg001]

This was reported in Ghost: https://github.com/TryGhost/Ghost/issues/7302

We could ask if the file exists, if yes we take the file for defaults and if not, defaults is empty.

See line: https://github.com/TryGhost/Ignition/blob/master/lib/config/index.js#L4

[kirrg001]

see #4 there is a TODO left, which needs to be solved as well (redesign how we load the config as function)

[kirrg001]

Here is my suggestion for how we can load different configs for different projects (this is tested)

Each project which uses Ignition and wants to use the advantage of the config, needs to do the following:

• create a config folder in your project /config
• create index.js /config/index.js

var config = require('ghost-ignition').loadConfig();
module.exports = config;

• so in each project you don't require Ignition config everywhere, you require your own config package, which acts like a wrapper to Ignition
• this has the big advantage of making custom changes in the project config wrapper

In ignition we will change the following:

• i will create a /config/load.js file
• as Ignition is also a project, we do the same in our /config/index.js - we call loadConfig
• i will use caller-id npm package, which is easier then the code i wrote for getting the parent/caller module

[ErisDS]

The main purpose of Ignition is to have a set of shared files so that we don't have to duplicate the creation & maintenance of files in each project. If we have to make a config folder / file in every project, that removes part of the purpose.

Also, if we have to include a file/folder, then requiring the config module in each project requires figuring out the paths again, like require('../../../../afas/adsdas/config') which I was really, really looking forward to not having to do in Ghost (and enjoying not doing in other projects).

I preferred the previous suggestion of having the require be require('ghost-ignition').config() everywhere the same as we do require('ghost-ignition').debug('something:here').

I tried to explain this earlier, but I think that Ignition has a few patterns in it which were done for convenience rather than being correct, and that we need to fix those bad ideas.

I think the usage of config.example.json as the defaults is a bad pattern. We did this in a few small internal projects where it was fine & made sense. Ghost will have defaults.json and overrides.json... it will also have a config.example.json but I think using that file is a "bad thing"™

In short, I think we need to ignore the assumption of ignition, figure out what config looks like for Ghost, and then figure out how to generalise that.

[kirrg001]

If you always call require('ghost-ignition').config(), you would always load the config again from file system?And putting a caching logic into Ignition is not the purpose of ignition?

An approach of a wrapper is always a good coding pattern. Just because we are outsourcing logic into a shared folder, that doesn't mean that we are now restricted to not using a config folder at all.

Beginning with the approach i have posted, we can start using nconf in Ghost. And the one who is using Ignition can still decide to not create a folder, but to load the config in a boot file. That is totally up to the user.

If you have a different idea in your head, i am happy to hear about it.

[ErisDS]

I really think that this should be built directly in Ghost first without Ignition, and abstracted out later.

[sebgie]

So, I have been running into errors with our configuration in Ghost for the last week. Depending on how or where the configuration was loaded the results were different, settings were missing or had the wrong value.

Ghost uses nconf for configuration management now. Ghost also depends on gscan and passport-ghost. Both of these modules depend on ghost-iginition which automatically initializes it's own nconf module to handle config. What happens is that this sub-sub-dependency overwrites the Ghost configuration store and makes environment variables unusable and adds/overwrites some of our settings.

I see the following options to solve that problem: 1) namespace our configs and use a "unique" top-key Ghost config:

    ghost: {
        ...
    }
}

gscan config:

    gscan: {
        ...
    }
}

This should still load the config twice but would fix the problem of overwriting values.

2) remove nconf from ghost-ignition

That means that all projects that depend on the module would need updating.

3) change ghost-ignition to not load the config module automatically

X-post from Slack and option 3) sounds similar to what is discussed here :).

[kirrg001]

As far as i understand, your comment @sebgie is not related to this issue. Maybe we should open another Github issue.

Ghost and Ghost-Ignition use nconf. Right now both operate on the same nconf provider instance (see https://github.com/indexzero/nconf/blob/master/lib/nconf.js).

So both should create their own nconf instance. As far as i understood, that needs to happen like this:

var nconf = require('nconf');
var config = new nconf.Provider();

config.env()
config.argv()

exports = config;

[sebgie]

I posted it here because it has to do with not loading nconf by default. Having a separate provider for ghost-ignition would mean that the stores won't override each other but if 2 configs use the same config property we would still override the values when using argv or env.

Is there any valid use case where a module would have it's own config? To not add too much complexity and prevent property collisions I would be in favour of only allowing the "main" service to use nconf. If a module requires nconf it would then operate on the parent configuration.

Ghost-ignition could still provide a prepared loading mechanism but not load a random config (even if it's just empty) by default.

[kirrg001]

but if 2 configs use the same config property we would still override the values when using argv or env.

An example is always better to understand 👍

[sebgie]

Assuming that all of these services use nconf or ghost-ignition a very simple example for this behaviour would be:

gscan requires a config for a port

config.development.json:
{
    port: 1234
}

passport-ghost also requires a config for a port

config.development.json:
{
    port: 4321
}

When I now run the following command to start Ghost, the above settings will be overwritten:

node index.js --port:3333

[kirrg001]

Sure, that's why we need something like var config = require('ghost-ignition').loadConfig(); as suggested on top of the issue. (which is point (3) on your comment)

But that's why we have the issue. What you have described was a different problem, which is solved with custom nconf providers.

[sebgie]

Sure, that's why we need something like var config = require('ghost-ignition').loadConfig(); as suggested on top of the issue. (which is point (3) on your comment)

Well, that's the reason why I posted the problem here ;-).

[ErisDS]

Is the problem here not, that gscan is screwing everything up by having both its app and the lib/bin part in one single module?

Would it be simpler, to split gscan into two repos?

It's a bit weird though, that even though the /app part of gscan doesn't get called by Ghost, there's interference between them?

[kirrg001]

Is the problem here not, that gscan is screwing everything up by having both its app and the lib/bin part in one single module?

Hmm maybe we all see different problems here. @ErisDS how about creating a separate issue with your concern of having app and lib in one single module in gscan?

My original comment in this issue was, that https://github.com/TryGhost/Ignition/blob/master/lib/config/index.js is a non dynamic js file - not functional.

When module A requires ignition and module B requires ignition, only one of them (the first), will initialise this file. And the second module B will get the config from module A. Because this file get's cached. I could also create an issue out of it.

Then we can close this issue.

[ErisDS]

They are the same problem.

When module A requires ignition and module B requires ignition, only one of them (the first), will initialise this file.

This only happens in the case of gscan?

[sebgie]

There are multiple problems which are separate but linked together in this issue now.

Is the problem here not, that gscan is screwing everything up by having both its app and the lib/bin part in one single module?

No, ghost-ignition initializes the config regardless if it is needed or not. That means that every module that depends on ghost-ignition will screw up the config. If we split up gscan, passport-ghost will still overwrite the config.

When module A requires ignition and module B requires ignition, only one of them (the first), will initialise this file. And the second module B will get the config from module A.

The config will be initialized 2 times in this case. They both operate on the same source file, but nconf.env, nconf.argv, ... will be executed multiple times. This is can be tested by adding a console.log() in Ghost to:

• /node_modules/gscan/node_modules/ghost-ignition/lib/config/index.js
• /node_modules/passport-ghost/node_modules/ghost-ignition/lib/config/index.js

The problem with double loading is partly solved by using separate instances but will overwrite config values if they contain properties that have the same name (see port example above).

This only happens in the case of gscan?

This happens for every module that requires ghost-ignition.

In my opinion the "best" solutionwould be to not load the configuration in ghost-ignition by default and initialize nconf only in apps and not libraries. If a library needs a config, it can still use nconf, but the parent application needs to do the initialization.

Examples:

• gscan-lib uses nconf for config.get() but doesn't initialize nconf
• gscan-app initializes nconf with nconf.env().argv() ...
• ghost initializes nconf and gscan-lib can then use the config initialized by ghost
• passport-ghost doesn't need a config and therefore doesn't load nconf

[ErisDS]

Oh, I had no idea that passport-ghost was using ghost-ignition! I thought gscan was the only submodule using it, and thus the root of all evil

gscan-lib uses nconf for config.get() but doesn't initialize nconf

I think the only place where gscan-lib uses config is actually in ignition's debug.js? Coupling the debug.js and config together was probably a really really bad idea. We can decouple them by getting rid of the alias concept, or by moving alias from being in config to using a custom key & value pair in package.json.

[ErisDS]

I am now 100% blocked on this issue, because Patronus + express-bookshelf-jsonapi = 💥

I will be tackling this today, unless @sebgie do you have a temporary workaround you are using in your projects?

[sebgie]

@ErisDS my workaround was to doctor ignition to not load the config. I commented out lines 25 - 33 in https://github.com/TryGhost/Ignition/blob/master/lib/config/index.js in the sub-sub dependencies.

[ErisDS]

Current use cases:

Case 1: Node service - uses ignition config

• express-bookshelf-jsonapi (ebja - needs a name 😂 ) - uses ignition, but not config

What happens: ignition tries to load config from ebja Ideally: top level node service gets config, nothing else should happen!

Case 2: Ghost - config via own-nconf

• gscan - use ignition config, but only for app, not when required as a module
• passport-ghost -- uses ignition, but not config

What happens/ideal case already written here: https://github.com/TryGhost/Ignition/issues/3#issuecomment-253789146

[kirrg001]

In my opinion, we have to put back getParentPath helper, which was reverted in https://github.com/TryGhost/Ignition/commit/075c87675b53afcd0764cf0481db1dfb9c95d76f. When i've added this function, i've tested all kinds of cases for installing Ignition as dependency and sub dependency, including the following case, which does not work anymore.

problem

• knex-migrator is sub dependency of Ghost
• Ignition is a dependency of knex-migrator and Ghost
• when you start Ghost, Ghost and knex-migrator initialise a logging instance, but Ignition reads for both the config of Ghost
• this results in e.g. Ghost defines a file transport for logging and both Ghost and knex-migrator create a log file (Ghost creates it in content/logs and knex-migrator in the root path of Ghost)

In my opinion Ignition should always only read the configuration files for the current parent and not of the current working directory.

[ErisDS]

@kirrg001 the commit 075c876 talks about there being no use-case, and this was the reason for reverting the code, which was, at the time, hard to reason about. Now that we have a use case, I think it will be easier to reason about the cases.

The key uses of ghost-ignition in subdependencies are:

Ghost -> gscan -> ghost-ignition (uses config, but only in the app, which Ghost doesn't use) Ghost -> passport-ghost -> ghost-ignition (does not use config) Internal Projects -> ebja -> ghost-ignition (does not use config) Ghost -> knex-migrator -> ghost-ignition (uses config via logging?)

As long as these all work, we should be safe 👍

[kirrg001]

@ErisDS Thanks. Will look at it tomorrow 👍

[kirrg001]

I will change the logging behaviour of knex-migrator. knex-migrator logging is only useful when starting using as CLI tool. And this can be stdout only for now. If using it within the application, knex-migrator returns a success or error response and based on it the caller can log or not log.