Feature/new config system (improved)

[mikkoi]

No description provided.

[xsawyerx]

If I understand correctly, the way you see it is:

• The App now uses Role::Config
• The role Role::Config has config_readers and config_files
• Each of the Config Reader classes consumes Role::ConfigReader
• Each of these classes also has its own config_files which they merge together
• The Role::Config will then merge these "merged" config files into its own config_files and its configuration itself, which then the App is able to use

Did I understand this right?

This makes sense, but it also suffers from the assumption that all configurations are files, so while it's more abstract it's - and I'm sorry to say this - not abstract enough.

The naming for Role::Config and Role::ConfigReader and ConfigReader gave me a bit of confusion trying to grok this.

I'm also missing the role HasLocation. I don't see where it went. Is it no longer necessary because the location is sent through the App to Role::Config?

I think this warrants further discussion because I don't think we should merge this when it's halfway between being too rigid and not being flexible enough. Once you could correct whatever I got wrong, I'm happy to propose an improved structure.

[xsawyerx]

It might be worthwhile exploring this warning.

[cromedome]

Seconded.

[xsawyerx]

"Users can control which B class to use when creating the config."

Same comment as above.

[xsawyerx]

Might be easier to write:

        my $runner_config;
        {
            no warnings 'once';
            $runner_config = defined $Dancer2::runner
                           ? Dancer2->runner->config
                           : {};
        }

I would still want to know if we could avoid that warning.

[mikkoi]

Reformatted.

[mikkoi]

If I understand correctly, the way you see it is:

The App now uses Role::Config
The role Role::Config has config_readers and config_files
Each of the Config Reader classes consumes Role::ConfigReader
Each of these classes also has its own config_files which they merge together
The Role::Config will then merge these "merged" config files into its own config_files and its configuration itself, which then the App is able to use

Did I understand this right?

Yes, precisely.
I made a conscious design decision to keep the changes to a minimum and maintain complete compatibility.
That is the reason why config_files is kept even though it doesn't make much sense in this new setup.
Instead of config_files we should have something like config_origin.
The same reason why the new solution isn't "abstract enough".

Again the same applies to HasLocation. Location is deduced once and then passed on to the config readers. Just like environment name.

[xsawyerx]

I think calculating location and environment only once and transferring it onwards is a good call. I like that.

I think the usage of roles here might be too much. How about:

• Dancer2::ConfigReader as an object holding configuration information.
• It has config_readers who all consume the Role::ConfigReader role.
• They can each have whatever attributes they want. The ConfigReader::File::Simple (or Config::Type::File::Simple) will have config_files as an attribute since it reads from [simple] configuration files. A different configuration might have elastic_params because it uses ElasticSearch to include configurations, for example.
• The App would create an instance of the ConfigReader object with the location and environment.
• The ConfigReader will determine which configuration reader it should use and instantiate them and use them to collect the configuration.

Two things left in mind:

• If you edit the value of the configuration readers to use using the set keyword, will we be able to use this by the time ConfigReader tries to instantiate configuration readers?
• We shouldn't (at least for now) support recursive levels of configuration readers, like "read the config file, then the config file contains other configuration readers, use them, etc." until we research how others handle this problem. (Thought: We do support local config files - so theoretically, we could use that second-level configuration part to hook into reading from non-configuration files...)

[mikkoi]

If we ditch config_files attribute, we create a non-compatible change.

The name is bad but I like the idea that the Config object would actually know where the configs come from, at least on a surface level (list of files or sources).

If you edit the value of the configuration readers to use using the set keyword, will we be able to use this by the time ConfigReader tries to instantiate configuration readers?

I don't think its possible. For the same reason I couldn't use hooks. It creates a dependency on user code before App is ready - and config should be read and ready before App is accessible for users.

We shouldn't (at least for now) support recursive levels of configuration readers, like "read the config file, then the config file contains other configuration readers, use them, etc." until we research how others handle this problem. (Thought: We do support local config files - so theoretically, we could use that second-level configuration part to hook into reading from non-configuration files...)

Agree. Too many moving parts for the moment.

[xsawyerx]

If we ditch config_files attribute, we create a non-compatible change.

Incompatible with what?

The name is bad but I like the idea that the Config object would actually know where the configs come from, at least on a surface level (list of files or sources).

Sorry, I don't understand your comment here.

[mikkoi]

Attribute config_files is part of the public interface, isn't it? If it isn't, then of course, it's not a compatibility issue.

Just thinking that I like the idea that the config readers maintain a list of files or sources where the config is read from.

[xsawyerx]

Attribute config_files is part of the public interface, isn't it? If it isn't, then of course, it's not a compatibility issue.

I can't imagine this being used for anything. The Config Reader is a role composed into classes. If someone is using it, that's not in a way that we should worry about officially supporting.

Just thinking that I like the idea that the config readers maintain a list of files or sources where the config is read from.

It makes sense if the specific sense of configuration as files, but in the abstract sense of configuration as values by keys, files are a secondary - and more important, optional - concept.

[cromedome]

Attribute config_files is part of the public interface, isn't it? If it isn't, then of course, it's not a compatibility issue.

The only thing someone could be using this for, I'd think, is logging the contents of what's there (and that is already happening on app startup). Maybe some devs got clever with the contents. We could remove it and see who complains, or for non-file based configs (etcd, Vault, wherever else you might pull config from) we could put a one-line description in about where the config came from. Personally, I'd rather remove it, make a developer release, document the removal, and see who complains.

Just thinking that I like the idea that the config readers maintain a list of files or sources where the config is read from.

As do I. I am not sure what I'd do with that info yet, but having it is intriguing. If only for some debugging purposes down the road.

[mikkoi]

I have changed all the small things and I removed the attributeDancer2::Core::Role::Config::config_files.

[mikkoi]

@xsawyerx
Did you really mean we should make so big change?
To abandon App having Config as a role, and instead instantiate ConfigReader?
It means we would need to implement the functionality to access configuration content in App and in ConfigReader.

Or else, we could redesign the whole thing so that Config and ConfigReader have nothing to do with each other, except that configuration is something that ConfigReader produces and then gives/assigns to Config.

It seems like a very big change.

[xsawyerx]

I do think we need to make such a big change and I don't think this big change is this "big." Of course, I could be wrong about that. (But it would still be the right thing to do.)

[mikkoi]

To be on the safe side, I rebased the branch from master. So I had to force push.

The new commits are:

• Create Role::HasEnvironment and tests
  839c0be
• Add more logging to ConfigReader::File::Simple
  b5afaa8
• Add isa type to HasLocation::location
  d159725
• Separate normalizers from Config to ConfigUtils
  94f21b7
• Split Role::Config to ConfigReader and Role::HasConfig
  ffd6c4d
• Add Mikko Koivunalho to contributors list
  c99b4b4

The main changes are:
There is now Role::HasConfig which has those parts of the old Role::Config which were about accessing and changing the config.
There is now Role::HasEnvironment to isolate the environment deducing code (we get env name from ENV vars, etc.)
ConfigReader is instantiated as an attribute to Dancer::Core::App.

[mikkoi]

The test t/issues/gh-634.t is failing because it uses config_location from Role::Config.
The test needs to be thought out again.

[mikkoi]

The tests now run.
@xsawyerx Do we need more fine tuning before merging?

[mikkoi]

Hi.
Can we open this discussion again?
I am all open for changing anything in the code.
I have been using this code in a website for two year now.

Thank you.

[cromedome]

@mikkoi I am not opposed to continuing a discussion of this, but I am booked solid until mid-May. Unsure if @xsawyerx or anyone else has time before then to work through this with you.

[mikkoi]

@cromedome @xsawyerx
Great.
I can wait.
Let's continue when you have the time.

[cromedome]

@mikkoi I am starting to take another look at this. Give me a little bit to work with it :)

[mikkoi]

@cromedome
Great.
Does it need rebasing?
Ready for changes.

[yanick]

A reeeally small nitpick, but I think it makes sense to have the delimiter be , instead of a space. That way you don't have to always quote the value of DANCER_CONFIG_READERS when it's multiple readers.

[mikkoi]

Good thing you noticed. I wonder why in the first I used a space. A comma is the correct delimiter to use.
I changed to comma and added a test to check the failure.

[yanick]

Woo! Thank you so much for the quick turnaround. After all that time, I wasn't expecting such speed. I'm going to do my best to finish my review this week and and have this moving forward. ❤️

[yanick]

This is excellent! Approved.

The only wish I had at the end of my review is that there would be a way to bootstrap config readers (so you can use a minimal yaml config file and specify your other ConfigReaders from there instead of having to use the environment variable), and I managed to do that very easily. I say we merge this as-is, and I'll submit a new PR for this extra bit of functionality.

Thanks a lot, mikkoi, for the PR, and for your patience. :-)

[cromedome]

@mikkoi this is great, and I am really happy with it! I would like to ask for one small change however:

Instead of the default being Dancer2::ConfigReader::File::Simple, could you make it Dancer2::ConfigReader::Config::Any? I feel that it is a more accurate name (there is nothing simple about our current config, and that is totally on me) that reflects the name of the underlying config module, and sets a good precedent for others who want to create future config readers for Dancer2.

I will approve and merge once this is done.

Great work, @mikkoi. Thanks for your patience and persistence!