Add require_environment setting

[yanick]

Port PerlDancer/Dancer#946 to Dancer2

[gideondsouza]

This one seems to have taken a back seat? Should I take this up next?

[mickeyn]

@gideondsouza it will be great, if you have the time :)

[gideondsouza]

hey @mickeyn

I am eager to get back on board but I'm a little out of touch (since it's been a while since I last saw the Dancer2 code)

I'll look at this today evening, I hope you guys will be open to a few questions 😄

[xsawyerx]

Very open! :)

[gideondsouza]

hey guys. So questions I have are:

1. Was the PR even merged in Improve hook/route handling when content is already set in response #946 on Dancer ? @yanick mentions it was merged but the PR shows CLOSED in red.
2. Excuse the possible stupidity but I'm assuming I should implement this inside Dancer2::Core::Role::ConfigReader.pm, but I'm a little perplexed where?:
   Maybe here while the config is being built:
   https://github.com/gideondsouza/Dancer2/blob/master/lib/Dancer2/Core/Role/ConfigReader.pm#L107

? I can check within the loop for the require_environment setting and call confess?

[veryrusty]

@gideondsouza It was merged: PerlDancer/Dancer@b5481de

(I haven't looked into the second item yet)

[gideondsouza]

@xsawyerx @veryrusty @yanick Sorry to poke you guys, what do you think?

Should it be implemented here? :
https://github.com/gideondsouza/Dancer2/blob/master/lib/Dancer2/Core/Role/ConfigReader.pm#L119

Along with the check that the file is readable (I wonder not readable also includes file not available) I will check for require _environment and confess() here?

[xsawyerx]

Makes sense to me. :)

[xsawyerx]

@gideondsouza are you claiming this issue? Just so we mark it and won't direct anyone else to work on it.

[gideondsouza]

Yes I am :)

On Sunday, June 8, 2014, Sawyer X [email protected] wrote:

@gideondsouza https://github.com/gideondsouza are you claiming this
issue? Just so we mark it and won't direct anyone else to work on it.

—
Reply to this email directly or view it on GitHub
#356 (comment).

[gideondsouza]

ok, so I ran into a problem:

I tried to implement this feature inside _build_config_files, I went and tried to mimic the way it was done with dancer1 however, when I try and do setting('require_environment') inside this function things started to fail. It turns into a bit of recursion. (Infact directly accessing $self->config runs into recursion)

This is when I analysed the code for a bit. The way it's structured here is:

• The config property (which is an alias for setting(..)) calls _build_config
• Which then calls the config_files property , which calls _build_config_files. This function returns the list of possible path+files

The _buid_config_files is written to ignore paths that are unreadable. But here is where I need to catch the case that a file was un-readable when the require_environment setting is true right?

The problem seems to be that all paths are determined together. This function maybe needs to be split ? So that the default config.XXX is loaded first, and if require_environment was set to true, then the environment config file is expected or the system croaks. But it seems like splitting the function into _build_main_config and _build_other_config will invite a lot of other change.

Sorry about the wall of text. I'd like some suggestions, or maybe I missed something here. Please let me know.

[xsawyerx]

Wall of text is just fine. :)

That does sound like a tricky problem. Maybe separating configuration reading from triggers?

[gideondsouza]

@xsawyerx not sure what triggers are...mind expanding on that? 😃

[xsawyerx]

Triggers are configuration options that initiate a subroutine. You can find those here:
https://github.com/PerlDancer/Dancer2/blob/master/lib/Dancer2/Core/App.pm#L58

[gideondsouza]

Ah I see. But I'm still not 100% here, I see that the triggers build up these four engines right qw<logger serializer session template>, but it doesn't seem to be reading any config?

Or are you referring to the fact that ConfigReader is a role for App.pm ?

I'm not seeing how the triggers end up calling config role methods and why this is related to the environment setting.

[gideondsouza]

@xsawyerx ok just to simplify my confusion I'm not seeing where the configuration reading is initiated as part of the triggers? App.pm does read the config but for it's own use.

Just poking you guys for good measure too @veryrusty @yanick @mickeyn because I want to get this done :)

[gideondsouza]

Hey guys, I was out on a little vacation and haven't seen this since.

Any updates on this? Could any of you guys give me more clues, tips, suggestions on how to go ahead with this.

In the meantime I was thinking of going and looking at any other issues I can tackle.

[DavsX]

Is this still a desired feature?

[DavsX]

PR: #780

[xsawyerx]

I'm sorry for all the delays with this ticket. I've been having a lot of thoughts regarding this. I'm thinking of maybe adding this to the concept of a strict mode. Then we could add more types of checks. Then again, this would be separate from a strict mode allowing you to remove old Dancer2 mannerisms in favor of new ones and strictures in the way your application works (expecting an environment file to be available, amongst other checks).

Would love to get more feedback about this.

[xsawyerx]

So, if I could delete all my previous comments on this issue and write this simple thing:
I feel like adding this additional configuration setting feels awkward.

Instead, what if we had "include" available as a configuration option, which would require us to include files? We could have it include any number using a glob (any that match) or filenames. If a file is mentioned by name (file-name, get it?), and it doesn't exist, it will crash. We make sure we never read files more than once, so if we already read the environment file (because it existed and we automatically do that), and you mentioned it as an include, it won't be read again. But, if it didn't exist (silently ignoring it), it would now try to find it and fail loudly.

This would be a generic solution which could be used not only to solve this problem, but to add additional include files (which we currently don't support).

[cromedome]

I'm very much in favor of an include directive. There are times I wish I could tuck some information away in another file separate from the ones Dancer uses, and I'd like to use them without creating my own YAML/JSON reader every time I need to access them.

How would this interact with the notion of strict_config? IIRC, strict_config would create an object from the config file. Would we leave anything included out of this object? Find a way to add functionality to the config object? Dump the existing config object and rebuild it with the new files? I can see use cases for all three, but my inclination would be don't alter the config object once it's instantiated.

👍 on the include idea though!

[GeekRuthie]

I'm looking at Dancer2::Core::Role::ConfigReader, and it looks like sub load_config_file is where an include-directive handler would need to go--I'm willing to take a swing at this, if there's still a desire for that.

One question, though--there's an ancient note in this sub # TODO handle mergeable entries, and I wanna make sure I understand that part of the problem--it seems like this would be a good time to clear up that tech debt, as well. By "mergeable entries," are we talking about correctly merging keys in the config, so that something like this will work:

In config.yml:

plugins:
   Foo:
      setting_1: "fizz buzz"

..then in an environment file:

plugins:
   Foo:
      setting_2: "foo bar baz"

As things are currently, I think that the environment would take precedence, and setting_1 would not be set. By "mergeable", do we mean that in this scenario, both setting_1 and setting_2 should be set?

[yanick]

By "mergeable", do we mean that in this scenario, both setting_1 and setting_2 should be set?

That'd be my understanding, yup!

... BWAHAHAHAHA I just scrolled all the way up and realized it's one of my tickets. 😂 Okay, I have to look at what the heck I was thinking about here.

[GeekRuthie]

The conversation moved quite a ways from your original post, @yanick. It kinda pivoted into an enhancement idea for an "include" directive in D2 configurations just a few comments ago, and that's where I am. It looks like your initial request has been done in some way. Everything from Sawyer's comment at #356 (comment) is kind of a different thing, and probably ought to be popped out to its own issue, maybe?

[cromedome]

Config, in general, needs some love. We have a PR #1637 that I am way, way overdue to look at. I'm starting to wonder if we should start grouping all the config issues together and start wading our way through what's best and what's not.