[asterisk-dev] [Code Review] 3148: res_pjsip: Config option to enable PJSIP logger at load time

[Kevin Harwell]

> On Jan. 21, 2014, 7:14 p.m., Paul Belanger wrote:
> > branches/12/contrib/ast-db-manage/config/versions/2fc7930b41b3_add_pjsip_endpoint_options_for_12_1.py, line 1
> > <https://reviewboard.asterisk.org/r/3148/diff/1/?file=53005#file53005line1>
> >
> >     You should be creating new alembic scripts not modifying committed ones. 
> >     
> >     This is going to breaks peoples systems that have already invoked the scripts.
> 
> Russell Bryant wrote:
>     right.  each script should stay unmodified.  any db schema changes should be a new script, which is a migration from the old schema to the updated one.  That way at each release, people can re-run alembic to apply all of the updates since the version of the schema they currently have.
> 
> Kevin Harwell wrote:
>     Correct and usually we would do just as mentioned.  In this case though the modified script has not been released (will be in 12.1).  Once released then it will stay unmodified and changes will go into a new script.
> 
> Paul Belanger wrote:
>     Well, the issue being you break peoples system that run the TIP of /branches/12, which you cannot control who as run the script.  So, the ideal way to handle it, is just leave the file as is and run 'alembic revision -m 'enable pjsip debug'.  This creates[1] fresh alembic scripts which you can move this code into, then you just build the upgrade / downgrade logic around it.
>     
>     Once the script has been committed to subversion, you have to assume somebody has run it, modifying it after the fact breaks these systems.
>     
>     [1] http://alembic.readthedocs.org/en/latest/tutorial.html#running-our-second-migration
> 
> Kevin Harwell wrote:
>     Agreed, and that is how I originally started out when addressing the addition of the "debug" option and would have done just that except unfortunately the "...options_for_12_1" script would fail as it was.  It was trying to add a column to a table that didn't exist.  It also had a problem with previously created enums when running against postgres.
>     
>     Since the script contains bugs it had to be modified and since it hasn't been officially released it made it easier to do so for this case.
> 
> Paul Belanger wrote:
>     so, if that is the case, you need to create a branch in alembic[1] to deal with the migration path.  If a user had run this migration, and they were at the latest changeset, the instructions would be to downgrade the script, reverting the database change.  This gets the users back to a sane configuration.
>     
>     We can then create a new path upgrade for the database, allow the user to select this new path.  More complicated, but doesn't leave people with broken databases.
>     
>     [1] http://alembic.readthedocs.org/en/latest/tutorial.html#working-with-branches

I think I might be missing something as I don't quite understand how the branching helps in this situation.  Alembic only allows a single migration path.  Branching occurs when two or more scripts attempt to work on the same path.  Alemic fails to process until the pathing issue is resolved.

In this case there is not a dual path/merge problem, but a problem in the source of the script itself.  I agree that we should usually avoid modifying the script, but the case where there is a bug in the script I am not sure there is a good way around it.


- Kevin


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


On Jan. 21, 2014, 5:01 p.m., Kevin Harwell wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3148/
> -----------------------------------------------------------
> 
> (Updated Jan. 21, 2014, 5:01 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-23038
>     https://issues.asterisk.org/jira/browse/ASTERISK-23038
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> Added a "debug" configuration option for res_pjsip that when set to "yes" enables SIP messages to be logged.  It is specified under the "system" type.
> 
> Also updated the alembic 12.1 script to include this option as well as a few others that were missing.  Also updated the "_adding_extensions" script in order to make the "id" column on the table a primary key because mysql needed it to be as such.
> 
> 
> Diffs
> -----
> 
>   branches/12/res/res_pjsip_logger.c 405906 
>   branches/12/res/res_pjsip/config_system.c 405906 
>   branches/12/res/res_pjsip.c 405906 
>   branches/12/include/asterisk/res_pjsip.h 405906 
>   branches/12/contrib/ast-db-manage/config/versions/581a4264e537_adding_extensions.py 405906 
>   branches/12/contrib/ast-db-manage/config/versions/2fc7930b41b3_add_pjsip_endpoint_options_for_12_1.py 405906 
>   branches/12/configs/pjsip.conf.sample 405906 
> 
> Diff: https://reviewboard.asterisk.org/r/3148/diff/
> 
> 
> Testing
> -------
> 
> Set the "debug" option in the pjsip.conf file and observed SIP debug messages on the console.  Also, tested the modified alembic scripts against postgres and mysql database servers.
> 
> 
> Thanks,
> 
> Kevin Harwell
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20140124/19f7bf71/attachment.html>