Re: [asterisk-dev] [Code Review] 4571: Add usegmtime option to cel_pgsql

2015-04-08 Thread Rodrigo Ramirez Norambuena

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4571/
---

(Updated April 8, 2015, 6:36 a.m.)


Status
--

This change has been marked as submitted.


Review request for Asterisk Developers.


Changes
---

Committed in revision 434284


Bugs: https://issues.asterisk.org/jira/browse/ASTERISK-23186

https://issues.asterisk.org/jira/browse/https://issues.asterisk.org/jira/browse/ASTERISK-23186


Repository: Asterisk


Description
---

Feature for added an option that lets you specify that the timestamps going 
into PostgreSQL from CEL log should be in GMT instead of local time.


Diffs
-

  /trunk/configs/samples/cel_pgsql.conf.sample 433966 
  /trunk/cel/cel_pgsql.c 433966 
  /trunk/CHANGES 433966 

Diff: https://reviewboard.asterisk.org/r/4571/diff/


Testing
---


Thanks,

Rodrigo Ramirez Norambuena

-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

Re: [asterisk-dev] [Code Review] 4571: Add usegmtime option to cel_pgsql

2015-04-07 Thread Rodrigo Ramirez Norambuena

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4571/
---

(Updated April 7, 2015, 10:54 p.m.)


Review request for Asterisk Developers.


Changes
---

Update patch for use gmt time on cel_pgsql


Bugs: https://issues.asterisk.org/jira/browse/ASTERISK-23186

https://issues.asterisk.org/jira/browse/https://issues.asterisk.org/jira/browse/ASTERISK-23186


Repository: Asterisk


Description
---

Feature for added an option that lets you specify that the timestamps going 
into PostgreSQL from CEL log should be in GMT instead of local time.


Diffs (updated)
-

  /trunk/configs/samples/cel_pgsql.conf.sample 433966 
  /trunk/cel/cel_pgsql.c 433966 
  /trunk/CHANGES 433966 

Diff: https://reviewboard.asterisk.org/r/4571/diff/


Testing
---


Thanks,

Rodrigo Ramirez Norambuena

-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

Re: [asterisk-dev] [Code Review] 4571: Add usegmtime option to cel_pgsql

2015-04-07 Thread Matt Jordan

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4571/#review15123
---

Ship it!


Ship It!

- Matt Jordan


On April 7, 2015, 8:54 p.m., Rodrigo Ramirez Norambuena wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviewboard.asterisk.org/r/4571/
 ---
 
 (Updated April 7, 2015, 8:54 p.m.)
 
 
 Review request for Asterisk Developers.
 
 
 Bugs: https://issues.asterisk.org/jira/browse/ASTERISK-23186
 
 https://issues.asterisk.org/jira/browse/https://issues.asterisk.org/jira/browse/ASTERISK-23186
 
 
 Repository: Asterisk
 
 
 Description
 ---
 
 Feature for added an option that lets you specify that the timestamps going 
 into PostgreSQL from CEL log should be in GMT instead of local time.
 
 
 Diffs
 -
 
   /trunk/configs/samples/cel_pgsql.conf.sample 433966 
   /trunk/cel/cel_pgsql.c 433966 
   /trunk/CHANGES 433966 
 
 Diff: https://reviewboard.asterisk.org/r/4571/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Rodrigo Ramirez Norambuena
 


-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

Re: [asterisk-dev] [Code Review] 4571: Add usegmtime option to cel_pgsql

2015-04-03 Thread Mark Michelson

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4571/#review15047
---


When I try to view the review, Review Board complains that it cannot display 
the changes because the diff uploaded did not apply cleanly. Please ensure that 
your working copy of the files being altered is up to date.

Also, if you're not already using it, there are some very useful command line 
tools you can use for uploading reviews to review board. See the Posting Code 
to Review Board section on this wiki page: 
https://wiki.asterisk.org/wiki/display/AST/Review+Board+Usage

- Mark Michelson


On April 3, 2015, 4:56 p.m., Rodrigo Ramirez Norambuena wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviewboard.asterisk.org/r/4571/
 ---
 
 (Updated April 3, 2015, 4:56 p.m.)
 
 
 Review request for Asterisk Developers.
 
 
 Bugs: https://issues.asterisk.org/jira/browse/ASTERISK-23186
 
 https://issues.asterisk.org/jira/browse/https://issues.asterisk.org/jira/browse/ASTERISK-23186
 
 
 Repository: Asterisk
 
 
 Description
 ---
 
 Feature for added an option that lets you specify that the timestamps going 
 into PostgreSQL from CEL log should be in GMT instead of local time.
 
 
 Diffs
 -
 
   /trunk/configs/samples/cel_pgsql.conf.sample 433966 
   /trunk/cel/cel_pgsql.c 433966 
   /trunk/CHANGES 433966 
 
 Diff: https://reviewboard.asterisk.org/r/4571/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Rodrigo Ramirez Norambuena
 


-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

Re: [asterisk-dev] [Code Review] 4571: Add usegmtime option to cel_pgsql

2015-04-01 Thread Mark Michelson

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4571/#review15023
---


Just commenting to say that aside from what Matt has already pointed out, I did 
not see anything else wrong with the change. So if you get his findings cleared 
up, this will get a Ship it! from me.

- Mark Michelson


On March 31, 2015, 1:19 a.m., Rodrigo Ramirez Norambuena wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviewboard.asterisk.org/r/4571/
 ---
 
 (Updated March 31, 2015, 1:19 a.m.)
 
 
 Review request for Asterisk Developers.
 
 
 Bugs: https://issues.asterisk.org/jira/browse/ASTERISK-23186
 
 https://issues.asterisk.org/jira/browse/https://issues.asterisk.org/jira/browse/ASTERISK-23186
 
 
 Repository: Asterisk
 
 
 Description
 ---
 
 Feature for added an option that lets you specify that the timestamps going 
 into PostgreSQL from CEL log should be in GMT instead of local time.
 
 
 Diffs
 -
 
   /trunk/configs/cel_pgsql.conf.sample 406488 
   /trunk/cel/cel_pgsql.c 406488 
   /trunk/CHANGES 406488 
 
 Diff: https://reviewboard.asterisk.org/r/4571/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Rodrigo Ramirez Norambuena
 


-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

Re: [asterisk-dev] [Code Review] 4571: Add usegmtime option to cel_pgsql

2015-03-31 Thread Matt Jordan

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4571/#review15004
---



/trunk/CHANGES
https://reviewboard.asterisk.org/r/4571/#comment25661

Two findings here:

1) The review shows a red blob. Extraneous white space should be deleted.

2) The formatting and wording here doesn't match other sections. I would 
use:

cel_pgsql
--
* Added a new option, 'usegmtime', which causes timestamps in CEL events
  to be logged in GMT.



/trunk/cel/cel_pgsql.c
https://reviewboard.asterisk.org/r/4571/#comment25662

There's no reason to initialize this to 0. static variables are allocated 
in the global data segment, and are always initialized to 0.

Technically, this applies to the 'connected' variable as well, but that's 
outside the scope of this review :-)



/trunk/cel/cel_pgsql.c
https://reviewboard.asterisk.org/r/4571/#comment25663

You actually don't need the variable. See comment below.



/trunk/cel/cel_pgsql.c
https://reviewboard.asterisk.org/r/4571/#comment25664

You don't need the newusegmtime variable here. Instead, this should simply 
be written as:

tmp = ast_variable_retrieve(cfg, global, usegmtime));
if (tmp) {
usegmtime = ast_true(tmp);
} else {
usegmtime = 0;
}

Note that this assumes that not setting the 'usegmtime' variable should 
also clear whatever the previous value to its default of False - which should 
probably be the case, since that is the default value.



/trunk/configs/cel_pgsql.conf.sample
https://reviewboard.asterisk.org/r/4571/#comment25665

Extraneous white space.


- Matt Jordan


On March 30, 2015, 8:19 p.m., Rodrigo Ramirez Norambuena wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviewboard.asterisk.org/r/4571/
 ---
 
 (Updated March 30, 2015, 8:19 p.m.)
 
 
 Review request for Asterisk Developers.
 
 
 Bugs: https://issues.asterisk.org/jira/browse/ASTERISK-23186
 
 https://issues.asterisk.org/jira/browse/https://issues.asterisk.org/jira/browse/ASTERISK-23186
 
 
 Repository: Asterisk
 
 
 Description
 ---
 
 Feature for added an option that lets you specify that the timestamps going 
 into PostgreSQL from CEL log should be in GMT instead of local time.
 
 
 Diffs
 -
 
   /trunk/configs/cel_pgsql.conf.sample 406488 
   /trunk/cel/cel_pgsql.c 406488 
   /trunk/CHANGES 406488 
 
 Diff: https://reviewboard.asterisk.org/r/4571/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Rodrigo Ramirez Norambuena
 


-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev