Code review comment for ~canonical-bootstack/charm-policy-routing:change-to-reactive-add-static-routing

Revision history for this message
David O Neill (dmzoneill) wrote :

Review:

From my review/understanding of this code, it seems that "lib_policy_routing.py" [1] is a forward port of the original code with a single "ExecStart" implementing routes.

Dikos lib_static_routing [2], supersedes this code providing support for multiple routes.

[1] also contains hard code magic numbers for table and priority, which serve very little purpose.

I would get rid of [1] altogether and provide support for table and priority in [2] dynamically.

Providing 2 different implementations in the same code base while is good for backwards capability, could provide unneeded confusion and increase our maintainability scope.

Further more [3] is solved by Dikos solution.

[1] https://git.launchpad.net/~canonical-bootstack/charm-policy-routing/tree/templates/service.j2?h=feature/reactive-upgrade
[2] https://git.launchpad.net/~canonical-bootstack/charm-policy-routing/tree/lib/lib_static_routing.py?h=feature/reactive-upgrade
[3] https://bugs.launchpad.net/charm-policy-routing/+bug/1792093

« Back to merge proposal