Re: [asterisk-dev] [Code Review] 4571: Add usegmtime option to cel_pgsql
--- 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
--- 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
--- 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
--- 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
--- 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
--- 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