Log Uniformization #1480
Log Uniformization #1480
Conversation
t/config.t
Outdated
| @@ -95,7 +95,7 @@ subtest 'Test configuration default modes' => sub { | |||
| # Test configuration generation with an unknown mode (should fallback to default) | |||
| $app = Mojolicious->new(); | |||
| $app->mode("foo_bar"); | |||
| OpenQA::ServerStartup::read_config($app); | |||
| OpenQA::FakeApp::read_config($app); | |||
There was a problem hiding this comment.
Can we not spread the name FakeApp then?
There was a problem hiding this comment.
...what about an OpenQA Base class then? :)
Codecov Report
@@ Coverage Diff @@
## master #1480 +/- ##
==========================================
+ Coverage 88.44% 88.47% +0.02%
==========================================
Files 107 106 -1
Lines 8074 8075 +1
==========================================
+ Hits 7141 7144 +3
+ Misses 933 931 -2
Continue to review full report at Codecov.
|
lib/OpenQA/FakeApp.pm
Outdated
| # is already sanitized. Otherwise we need to check | ||
| my $logfile | ||
| = catfile($self->log_dir, hostname() . (defined $self->instance ? "-${\$self->instance}" : '') . ".log"); | ||
| my ($logfile, $logdir, $level, $log) = ('', '', '', undef, ''); |
There was a problem hiding this comment.
i think this is not necessary
There was a problem hiding this comment.
Those are the default values.
For example the logdir variable. If the object isn't of type FakeApp, then the object doesn't have a logdir function. Since i'm setting that value in the if only for the objects FakeApp:
if ($self->isa('OpenQA::FakeApp')) {
$logdir = $self->log_dir;
$level = $self->level;
}
else {
$log = $self->log;
OpenQA::FakeApp::read_config($self);
$level = $self->config->{logging}->{level} || 'debug';
}
then i can use it later like:
if ($logfile && $logdir) {
my $logfile = catfile($logdir, $logfile);
$log = Mojo::Log->new(handle => *STDOUT, level => $self->level);
}
Or did i understood wrongly your comment?
There was a problem hiding this comment.
It's ok to use different variables - but defaults values looks really weird :)
Why can't just be:
my ($logfile, $logdir, $level, $log);
dasantiago
force-pushed
the
25958
branch
8 times, most recently
from
d4690ad to
39ecf01
Compare
lib/OpenQA/FakeApp.pm
Outdated
| } | ||
| else { | ||
| $log = $self->log; | ||
| OpenQA::FakeApp::read_config($self); |
There was a problem hiding this comment.
You don't need the full qualified name inside same class
dasantiago
force-pushed
the
25958
branch
8 times, most recently
from
d62f56c to
1b9b39c
Compare
lib/OpenQA/FakeApp.pm
Outdated
| else { | ||
| $log = Mojo::Log->new(handle => \*STDOUT, level => $level); | ||
| } | ||
| $log->format( |
There was a problem hiding this comment.
When logging to STDOUT/STDERR, output format should not contain time, see the portion you removed: 1b9b39c#diff-5289bf936364851cb95d4833773bce49L132 - Also see https://progress.opensuse.org/issues/13866
dasantiago
force-pushed
the
25958
branch
6 times, most recently
from
b4db9fe to
699d922
Compare
dasantiago
force-pushed
the
25958
branch
11 times, most recently
from
ec87f0f to
c8a1aca
Compare
Sometimes it's required that we are able to log text to multiple
files. Currently the default OpenQA::Setup::app only logs to the
destination specified in the config files.
With these changes if the developer wants or needs to log to
another file/destination, just needs to call the normal logging
functions with an extra parameter, which is the name of the
"channel". Example:
> log_debug($message, 'this is name of channel');
This will work as long as the "channel" already exists. Otherwise,
it will use the default logging mechanism.
To create a new "channel" just invoke add_log_channel with the
**id** as first parameter. The rest of the parameters are the
same as the ones required for Mojo::Log. Example:
> add_log_channel('channel name', path=>'test.log', level=>'debug')
> unless get_channel_handle('autoinst');
This will create a Mojo::Log object that will be associated with
the name 'channel name'
|
|
commit 31ec90c Merge: 9575846 4f54690 Author: Stephan Kulow <[email protected]> AuthorDate: Fri Nov 10 10:15:42 2017 +0100 Commit: GitHub <[email protected]> CommitDate: Fri Nov 10 10:15:42 2017 +0100 Merge pull request #1480 from dasantiago/25958 Log Uniformization
commit 31ec90c Merge: 9575846 4f54690 Author: Stephan Kulow <[email protected]> AuthorDate: Fri Nov 10 10:15:42 2017 +0100 Commit: GitHub <[email protected]> CommitDate: Fri Nov 10 10:15:42 2017 +0100 Merge pull request #1480 from dasantiago/25958 Log Uniformization
commit 31ec90c Merge: 9575846 4f54690 Author: Stephan Kulow <[email protected]> AuthorDate: Fri Nov 10 10:15:42 2017 +0100 Commit: GitHub <[email protected]> CommitDate: Fri Nov 10 10:15:42 2017 +0100 Merge pull request #1480 from dasantiago/25958 Log Uniformization
commit 31ec90c Merge: 9575846 4f54690 Author: Stephan Kulow <[email protected]> AuthorDate: Fri Nov 10 10:15:42 2017 +0100 Commit: GitHub <[email protected]> CommitDate: Fri Nov 10 10:15:42 2017 +0100 Merge pull request #1480 from dasantiago/25958 Log Uniformization
commit 31ec90c Merge: 9575846 4f54690 Author: Stephan Kulow <[email protected]> AuthorDate: Fri Nov 10 10:15:42 2017 +0100 Commit: GitHub <[email protected]> CommitDate: Fri Nov 10 10:15:42 2017 +0100 Merge pull request #1480 from dasantiago/25958 Log Uniformization
|
why do I keep referencing this one? :) |
|
could this be the cron builds of travis? |
|
Yup, it's the cron from travis... Since the line at the end of the documents keep changing, there's a new update :P |
|
can we get rid of this timestamp? |
|
Is this PR the one that also changed the timestamp format of os-autoinst? It seems we have more text in one line but which is not useful for debugging a test, e.g. [:] but OTOH are missing the subsecond precision output in the timestamp which is crucial to understand some test issues. Could you please take care to bring that back? |
|
no, but you found the proper one in os-autoinst yourself |
This change will remove the ServerStartup, which is only used for log
configuration. The FakeApp was extended to support the ServerStartup
functionality.