Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

confdir config option not actually used anywhere #919

Open
chrisspang opened this issue Jun 1, 2015 · 15 comments
Open

confdir config option not actually used anywhere #919

chrisspang opened this issue Jun 1, 2015 · 15 comments
Labels

Comments

@chrisspang
Copy link

I was expecting to be able to get Dancer2 to load a config from a specific directory by using the 'confdir' option. It is documented in Dancer2::Config:

DANCER_CONFDIR
 Sets the configuration directory.

 This correlates to the "confdir" config option.

It's not used anywhere in code, though.

ConfigReader.pm seems to deduce the configuration directory using some horrible code in HasLocation.pm which just trawls the file-system looking for a .dancer file (!) or a diretory with a lib and bin subdirectory (!!).

ConfigReader.pm:

has config_location => (
    is      => 'ro',
    isa     => ReadableFilePath,
    lazy    => 1,
    default => sub { $ENV{DANCER_CONFDIR} || $_[0]->location },
);

I'd rather not have my apps just use the first config they come across on the filesystem - can this be changed to use a specified directory?

@xsawyerx
Copy link
Member

This relates to #408.

We should definitely support confdir again.

@vorobeez
Copy link

Can i take this bug?

@veryrusty
Copy link
Member

@vorobeez go for it! 😄

@xsawyerx
Copy link
Member

What we are looking for is support for the configuration option confdir which will allow someone to control where the configuration will be loaded from. Come to think of it, this might not be possible. There are two situations in which someone uses it:

  1. On the command line
  2. In the app: use Dancer2; set confdir => '...';

The first isn't supported since the command line is now plackup. We might add more options to that, but for now it's not supported.

The second is probably too late to set it, since as soon as you call use Dancer2 the configuration is loaded. I might be wrong on that, but that's the first thing you'll need to check. Then, if it's loaded lazily, we can add the support. If it's not, maybe we can load things more lazily and then add support.

@vorobeez
Copy link

@xsawyerx, thank you for explanations.
I'm going to code review. (actually, i'm started reviewing ConfigReader.pm).
I wanted to say that my code review may take week or more, is it not bad?
(i'm not familiar with Moose).

@xsawyerx
Copy link
Member

@vorobeez Not a problem at all. :)

@cromedome
Copy link
Contributor

@vorobeez how's it going with this? Anything we can help with? Please let me know! :)

@rleir
Copy link

rleir commented Oct 9, 2015

Can we put something in bin/app.psgi to set the confdir option?
Or do it via an env variable. Both would be a bit confusing.
Or a symbolic link from config.yml to /etc/myapp/config.yml so the confdir variable is not needed.

@cromedome
Copy link
Contributor

You can set DANCER_CONFDIR. Looking through Dancer2::Core::Role::ConfigReader, this seems supported already.

I like the idea of use MyApp; set confdir => ... in my app.psgi, but I don't know how feasible that is yet. Still getting the hang of some things.

@vorobeez
Copy link

@cromedome, hello. Sorry, i didn't have so much time to investigate this bug. Now i am reviewing Dancer2::Core:App.pm. Give me, please, one week to offer some solution.

@cromedome
Copy link
Contributor

No problem! Not trying to rush you, I just wanted to make myself available if you needed anything :)

On Oct 11, 2015, at 2:22 PM, Andrew Firsov notifications@github.com wrote:

@cromedome, hello. Sorry, i didn't have so much time to investigate this bug. Now i am reviewing Dancer2::Core:App.pm. Give me, please, one week to offer some solution.


Reply to this email directly or view it on GitHub.

@vorobeez
Copy link

@cromedome, hello again. I have some questions. Maby you will answer to me?

  1. I didn't use Moo before. Will BUILD method called when you create instance of some class? (e.g. Dancer2::Core:App).
  2. I don't understand at all how Dancer2 starts. For instance i have that code:
#!/usr/bin/perl

use warnings;
use strict;
use Dancer2;

get '/' =>  sub {
    send_file('index.html');
};
dance;

What will happen after calling dance method? And when Dancer2 will call import method from Dancer.pm?

@rleir
Copy link

rleir commented Nov 6, 2015

The 'dance' keyword does not seem to be necessary, in fact my tests fail when it is in my code. This is off-topic; I recommend the dancer-users mailing list.

Did my comment on Oct 8 cover the confdir issue? If so, let's close this issue and focus on #408 . Thanks -- Rick

@cromedome
Copy link
Contributor

Andrew/Rick,

Andrew: sorry for the delay in responding to this.

I have actually started a somewhat ambitious project to deal with the list of config related issues… covering confdir, issue 408, and more.

I should have a branch pushed on my fork sometime this weekend, I hope, and you can both see what I am working towards. Your feedback/input/patches will certainly be welcome.

Rick: I am not as convinced that ConfigReader is more complex than it needs to be. Let me get my branch cleaned up and pushed and let’s discuss some more?

Thanks!
Jason

On Nov 6, 2015, at 7:34 AM, Rick Leir notifications@github.com wrote:

The 'dance' keyword does not seem to be necessary, in fact my tests fail when it is in my code. This is off-topic; I recommend the dancer-users mailing list.

Did my comment on Oct 8 cover the confdir issue? If so, let's close this issue and focus on #408 . Thanks -- Rick


Reply to this email directly or view it on GitHub.

@schaiba
Copy link

schaiba commented Feb 25, 2024

Hello, are there any updates regarding this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants