Configuration: audit, clean, extend & improve

[ErisDS]

As the refactor of config is done, and we've had a chance to use it for a little while, it would be good to revisit it again before the end of alpha. There are a couple of outstanding todos from #6982 plus I'd like to look at what config options we provide. Do they make sense? Are there some things which should not be config? Some things which should be and aren't?

Things that definitely need review/audit:

• IMPORTANT: reconsider new config options from https://github.com/TryGhost/Ghost/pull/7929⭐️ ⭐️ ⭐️ ⭐️ ⭐️
  - [ ] our use of paths in overrides.json need's more context
  - [ ] revisit paths like /ghost/ and /ghost/api/<verison> need's more context
• our use of the url and related config options like forceAdminSSL (+ redirect option).
  • Required: support for adminUrl like we use on Ghost(Pro)
  • Drop handling on Ghost(Pro)
• should route keywords be configuration? See Make route keywords configureable #8492
• configuration options for caching (raised in Support configuration of cache-control max-age #7273)
• documentation & implementation of compress config differs (docs say compress implementation looks for server.compress).
• imageRelPath https://github.com/TryGhost/Ghost/blob/master/core/server/config/utils.js#L19
• revisit all checks against config.get('env') and see if they can be moved to configuration E.g. https://github.com/TryGhost/Ghost/pull/7544/files

- [ ] catch the error thrown from nconf, and rethrow it in a prettyier, clearer, more Ghost-like fashion (see #7488 (comment)):

/Users/hannah/Ghost/node_modules/nconf/lib/nconf/stores/file.js:160
    throw new Error("Error parsing your configuration file: [" + this.file + ']: ' + ex.message);
    ^

Error: Error parsing your configuration file: [/Users/hannah/Ghost/config.development.json]: Unexpected token d
...

Anyone who has something else they'd like reconsidered, please add a comment!

TODOs from #6982:

• revisit the use of keys with nconf's .file()
• remove the default database config, add client to the config.development.json file

TODO's from #7692

• remove config.set('theme:..`), look at the TODO written in the PR

Issue fixes

• 🐛 (?) config: database merging #7500
• config: it's not possible to override defaults via argv  #7492

[ErisDS]

I just added a todo here to revist all of our checks against config.get('env') to see if some can be moved to be config. E.g. the checks which prevent rpc pings unless we're in production could be configuration overrides added to the defaults or to the default testing & dev envs.

[kirrg001]

E.g. the checks which prevent rpc pings unless we're in production could be configuration overrides added to the defaults or to the default testing & dev envs.

Can you show an example?maybe pseudo code?

[ErisDS]

Instead of doing if (config.get('env') === 'production') we would have if (config.get('enableRPCPings')) or something.
I'm just suggesting that where possible, we should use a specific/named config variable

[kirrg001]

Agree, if a unit can be enabled or disabled per product definition, then it would makes sense 👍

[kirrg001]

/Users/hannah/Ghost/node_modules/nconf/lib/nconf/stores/file.js:160
throw new Error("Error parsing your configuration file: [" + this.file + ']: ' + ex.message);
^
Error: Error parsing your configuration file: [/Users/hannah/Ghost/config.development.json]: Unexpected token d
...

If a custom config file can't be parsed, then Ghost won't be able to print a nice error, because logging depends on config! If config can't be load, logging can't be initialised. Even if we force logging to initialise with a default config, we would have to add try/catch statements to the logging adapter and to the bootstrapping of Ghost, because config loads synchronously! Or we could load config asynchronous. But all of this because we can't parse a JSON file?

I suggest that, as we have a config validation issue, which is committed as must have for 1.0.0, we only care in Ghost-CLI and show the user that his custom config file is invalid and that's why he can't start Ghost.

[ErisDS]

Couple of notes / thoughts as I'm moving around:

• reconsider: in overrides.json, times key needs to be changed to something like scheduling as it is part of that aspect of Ghost.
• minifyAssets needs revisiting as it is doesn't do anything really
• internalApps should possibly be apps.internal?
• think about content paths:
  • I would like to make it possible at some point soon (not necessarily for 1.0) to change the path for where themes are stored - this would be the only available customisation, no adapters. we will keep it in mind
  • I would like adapters to all live in /content/adapters by default, rather than having separate folders.

[kirrg001]

@ErisDS How about creating smaller config issues, assign to the milestone if they are required for the beta and close this one?

[kirrg001]

I just went over the last tasks from the issue.

in overrides.json, times key needs to be changed to something like scheduling as it is part of that aspect of Ghost.

We are using the times object for multiple purposes. e.g. scheduling and image size timeout. The idea was to start a collection with every possible static time configuration.

minifyAssets needs revisiting as it is doesn't do anything really

Was already removed and replaced by useMinFiles, see a413d70.

internalApps should possibly be apps.internal

yeah can do that in case we would like to add apps.external later 👍

I would like adapters to all live in /content/adapters by default, rather than having separate folders.

done already, see 1b965fa and be5b584

I would like to make it possible at some point soon (not necessarily for 1.0) to change the path for where themes are stored - this would be the only available customisation, no adapters.

We will keep that in mind.

should route keywords be configuration

Probably yes. I've tested a case a couple of days ago to change /subscribe to /something - it worked pretty good. But when looking at the priority and the effort (testing every single case), i would like to raise a (community) issue for now - can be also done by us after the beta. I will raise an issue for now.

configuration options for caching

Was partially done for theme and admin. I will also add it for the cache control.

our use of paths in overrides.json
revisit paths like /ghost/ and /ghost/api/

Not sure what to reconsider here 🤔 This needs more context.

[kirrg001]

closed via #8491

[ErisDS]

I would like to make it possible at some point soon (not necessarily for 1.0) to change the path for where themes are stored - this would be the only available customisation, no adapters.

We will keep that in mind.

our use of paths in overrides.json
revisit paths like /ghost/ and /ghost/api/

Not sure what to reconsider here 🤔 This needs more context.

To clarify what I meant by this (sorry, I realise there is not anywhere near enough information), I was thinking about the fact that we have a huge block of "paths" in overrides.json, however:

The admin path, /ghost/ and the api path /ghost/api/ are not there, instead they are littered throughout the code base. This is a cleanup thing, not a breaking change so doesn't need to be looked at urgently.

Meanwhile with regard to the path for themes, that is derived from the ONLY path that is allowed to be overridden, because in defaults.json we have:

 "paths": {
        "contentPath": "content/"
    },

However, themePath is not something that can be overridden on its own, because it is calculated based on content path, using the getContentPath() function. I left an item here to reconsider this - is this the correct thing to do, or should all the individual content paths be items in the defaults.json that can be overridden individually? E.g.

 "paths": {
        "imagePath": "content/images",
        "themesPath": "content/themes"
    },

Again, don't think it's urgent, just wanted to clarify because I saw this. Let me know if it makes sense now?

[kirrg001]

👍 Thank you!

I agree to the custom theme and image paths. We can add this later as a none breaking change and support both notations. You either specify a contentPath or override the content path by e.g. contentImagePath - if only one option is specified, it's possible to detect that.