Dbixdpnc #165

psladek · 2015-02-06T07:58:20Z

action #5852 - remove DBIx::Class::Schema::Config

loading database.ini via Config::IniFiles,
moving connect_db() moved from Utils.pm to Schema.pm

coolo · 2015-02-06T08:48:30Z

the test suite protests heavily

- database config loading via Config::IniFiles - removed deps on Config::Tiny and DBIx::Class::Schema::Config - connect_db() moved from Utils.pm to Schema.pm - added enviromental variable OPENQA_DATABASE_CONFIG to specify path to database ini file - used this for tests via OpenQA::Test::Case module

Conflicts: lib/OpenQA/Schema/Schema.pm

psladek · 2015-02-12T17:45:51Z

tests access to config file should be fixed now

coolo · 2015-02-12T20:21:27Z

lib/OpenQA/Schema/Schema.pm

+        my %ini;
+        #        print("ENV{OPENQA_DATABASE_CONFIG}=$ENV{OPENQA_DATABASE_CONFIG}\n");
+        tie %ini, 'Config::IniFiles', ( -file => $ENV{OPENQA_DATABASE_CONFIG} || $configname );
+        $schema=__PACKAGE__->connect($ini{$mode});


as I just documented the use of OPENQA_CONFIG (see #171), why did you change?

oh, I see, sorry, did not noted your change before pull request. I did change it because from the code I had an impression that OPENQA_CONFIG is only for openqa.ini. It was also originaly done because of the test scripts, which, if in deeper level, could not find the database.ini file ../lib (../../lib was needed). Since in the OpenQA::Test::Case was already a OPENQA_CLIENT_CONFIG variable, it seemed natural to have another independent path for database.ini, in something like OPENQA_DATABASE_CONFIG.

If you think it is better as it was described in the documentation, I can change it to derive from OPENQA_CONFIG, but I still think, since we have already independent OPENQA_CLIENT_CONFIG, the database.ini may be justified to have its own env variable.

I would rather drop the OPENQA_CLIENT_CONFIG than having a variable per file

That's also possibility, but since it will restrict all ini files in one directory (assuming same logic for workers.ini as well), we will need to copy/link openqa.ini and database.ini into t/data directory, and also move lib/openqa.ini and lib/database.ini into etc/openqa. Is that ok to do so?

also this will force all configs except openqa.ini to have always same names, maybe we should make this so also for openqa.ini and put only the dirname into the OPENQA_CONFIG ?

copying, or more likely linking to t/data is needed because of t/data/client.conf. And since all cfg files have to have same directory path now, they must be present there too.

Second comment: I did not say same dirname, but same config name. While you can specify whole pathname (dir+file name) in OPENQA_CONFIG for openqa.ini, the other files have fixed names (defaults, and same directory as OPENQA_CONFIG). So I think it would be better to have all configs on same standing.
Deciding at runtime seems possible, but not so nice, since one have to check whether OPENQA_CONFIG is file or directory everytime any config file needs to be loaded. On the other hand it would make it backward compatible... But then it also dangerously resemble the idea of combining OPENQA_CONFIG and OPENQA_DATABASE_CONFIG, not sure what coolo would think about that, concerning documentation and such... :)

copying, or more likely linking to t/data is needed because of t/data/client.conf. And since all cfg files have to have same directory path now, they must be present there too.

Yeah, but is it needed? I don't think they are used anywhere in tests.

Second comment: I did not say same dirname, but same config name. While you can specify whole pathname (dir+file name) in OPENQA_CONFIG for openqa.ini, the other files have fixed names (defaults, and same directory as OPENQA_CONFIG). So I think it would be better to have all configs on same standing.

Aha, I understood that as a same name (ie. everything called openqa.ini) which didn't make sense to me. Fixed name it is then.

Deciding at runtime seems possible, but not so nice, since one have to check whether OPENQA_CONFIG is file or directory everytime any config file needs to be loaded.

AFAIK they are loaded once on startup, I don't see where the problem is.

Yeah, but is it needed? I don't think they are used anywhere in tests.

yes, it is needed, for example t/api/06-products.t needs database.ini

AFAIK they are loaded once on startup, I don't see where the problem is.

It could of course be done, but I think it is still clearer, in code, documentation and usage to have only one meaning for OPENQA_CONFIG. The only problem is backward compatibility..

Yeah, but is it needed? I don't think they are used anywhere in tests.

yes, it is needed, for example t/api/06-products.t needs database.ini

Ah right, by the very same thing you are trying to get rid of I suppose
(Schema::Config). I wasn't paying attention and it is hidden deep. :)
I expected tests to be self contained and if needed, database.ini would be
already there.

Not sure what you mean by they would already be there, I think simply linking the ini files from etc/openqa (where lib/database.ini and lib/openq.ini would be moved too) would make tests work as they do now, which means to also test the project's ini files validity. If later some specific test needs special ini file(s) just for itself, it can have them in some another t/data like directory (again linking the ones that need not to be specific).

psladek · 2015-02-13T13:56:21Z

Also, what do you think about this solution:
OPENQA_CONFIG is set, OPENQA_DATABASE_CONFIG unset : use path from OPENQA_CONFIG
OPENQA_DATABASE_CONFIG set : use path from OPENQA_DATABASE_CONFIG
?

coolo · 2015-02-13T15:46:55Z

whatever you do: document it. and as you document it, you will notice how silly it is :)

psladek · 2015-02-13T19:13:44Z

I don't really think so, but in any case, I think I'd rather spare myself the effort :)

- moved openqa.ini and database.ini lib -> etc/openqa

Conflicts: script/worker

lnussel · 2015-02-20T09:22:35Z

lib/OpenQA/Client.pm

@@ -37,7 +36,10 @@ sub new {
    }

    if ($args{api}) {
-        for my $file ($ENV{OPENQA_CLIENT_CONFIG}||undef, glob('~/.config/openqa/client.conf'), '/etc/openqa/client.conf') {
+        my @cfgpaths=('~/.config/openqa', '/etc/openqa');
+        @cfgpaths=($ENV{OPENQA_CONFIG},@cfgpaths) if defined $ENV{OPENQA_CONFIG};


you can use unshift here

I know. I am more used to this form though, and in general find it a bit more readable and self explaining.
Can change it if you insist though...

coolo · 2015-02-20T10:45:19Z

fine with me now

coolo · 2015-02-20T10:53:24Z

I tried to rebase this, but it's too long history, so merging as is

Dbixdpnc

psladek added 2 commits February 12, 2015 18:10

Merge branch 'master' into dbixdpnc

e4c65a5

Conflicts: lib/OpenQA/Schema/Schema.pm

psladek force-pushed the dbixdpnc branch from 4b85162 to e4c65a5 Compare February 12, 2015 17:28

coolo reviewed Feb 12, 2015
View reviewed changes

psladek added 2 commits February 19, 2015 18:21

action #5852 - all cfg file dir in OPENQA_CONFIG

64d35cd

- moved openqa.ini and database.ini lib -> etc/openqa

Merge branch 'master' into dbixdpnc

f9b51e0

Conflicts: script/worker

lnussel reviewed Feb 20, 2015
View reviewed changes

psladek added 2 commits February 20, 2015 11:04

fix: expansion of home path via glob

7f2d514

Merge branch 'master' into dbixdpnc

b4b9bb2

coolo added a commit that referenced this pull request Feb 20, 2015

Merge pull request #165 from psladek/dbixdpnc

ef41072

Dbixdpnc

coolo merged commit ef41072 into os-autoinst:master Feb 20, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dbixdpnc #165

Dbixdpnc #165

psladek commented Feb 6, 2015

coolo commented Feb 6, 2015

psladek commented Feb 12, 2015

coolo Feb 12, 2015

psladek Feb 13, 2015

coolo Feb 13, 2015

psladek Feb 13, 2015

psladek Feb 13, 2015

psladek Feb 18, 2015

aaannz Feb 18, 2015

psladek Feb 18, 2015

aaannz Feb 18, 2015

psladek Feb 18, 2015

psladek commented Feb 13, 2015

coolo commented Feb 13, 2015

psladek commented Feb 13, 2015

lnussel Feb 20, 2015

psladek Feb 20, 2015

coolo commented Feb 20, 2015

coolo commented Feb 20, 2015

Dbixdpnc #165

Dbixdpnc #165

Conversation

psladek commented Feb 6, 2015

coolo commented Feb 6, 2015

psladek commented Feb 12, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

psladek commented Feb 13, 2015

coolo commented Feb 13, 2015

psladek commented Feb 13, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coolo commented Feb 20, 2015

coolo commented Feb 20, 2015