----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard-fedoraserver.rhcloud.com/r/17/#review57 ----------------------------------------------------------- We do need an automated test suite sooner rather than later… these kinds of oversights should be caught by code, not by manual review. config/roles/testrole/role.py <http://reviewboard-fedoraserver.rhcloud.com/r/17/#comment43> Silently returns None on an unmatched property config/roles/testrole/role.py <http://reviewboard-fedoraserver.rhcloud.com/r/17/#comment44> Silently returns None on an unmatched property src/rolekit/server/rolebase.py <http://reviewboard-fedoraserver.rhcloud.com/r/17/#comment45> 1. Because the method returns None, this doesn’t fire. 2. Blind “except:” is evil. Either define a specific exception type for this, or at the very least an unique object: PropertyNotRecognizedValue = object(); globally, then v = x.do_get_property(…) if v is not PropertyNotRecognizedValue: return v src/rolekit/server/rolebase.py <http://reviewboard-fedoraserver.rhcloud.com/r/17/#comment46> 1. Because the method returns None, this doesn’t fire. 2. Blind “except:” is evil. Either define a specific exception type for this, or at the very least an unique object: PropertyNotRecognizedValue = object(); globally, then v = x.do_get_property(…) if v is not PropertyNotRecognizedValue: return v - Miloslav Trmac On Čec. 21, 2014, 3:16 odp., Thomas Woerner wrote:

The concern is about completely unknown setting names; the final do_get_property() just returns None for them. OK, but the code still doesn’t work as written because it just tries to return None and UNKNOWN_SETTING is never returned. - Miloslav ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard-fedoraserver.rhcloud.com/r/17/#review57 ----------------------------------------------------------- On Čec. 22, 2014, 5:35 odp., Thomas Woerner wrote:

----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard-fedoraserver.rhcloud.com/r/17/ ----------------------------------------------------------- (Updated July 21, 2014, 9:59 p.m.) Status ------ This change has been discarded. Review request for RoleKit Mailing List, Stephen Gallagher and Thomas Woerner. Repository: rolekit Description ------- Simplifies the role interface Role cleanup by dropping "failonthis" setting Diffs ----- config/roles/testrole/role.py 2f077c62b4a8027e7783a2e08c84bc9c9715393e src/rolekit/server/rolebase.py 50b5685a038789d02d3f3b0451f5edaecc187964 Diff: http://reviewboard-fedoraserver.rhcloud.com/r/17/diff/ Testing ------- Thanks, Thomas Woerner

----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard-fedoraserver.rhcloud.com/r/17/#review109 ----------------------------------------------------------- config/roles/testrole/role.py <http://reviewboard-fedoraserver.rhcloud.com/r/17/#comment82> Shouldn't this be self.do_get_dbus_property()? - Stephen Gallagher On July 22, 2014, 3:35 p.m., Thomas Woerner wrote:

It is not in do_get_dbus_property. If property is available in dbus.service, then we are using the new property model (latest Fedora additions to dbus-python), then this method is needed. Otherwise the old model is needed and done automatically in the Get method in RoleBase. - Thomas ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard-fedoraserver.rhcloud.com/r/17/#review199 ----------------------------------------------------------- On July 22, 2014, 3:35 p.m., Thomas Woerner wrote:

----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard-fedoraserver.rhcloud.com/r/17/#review255 ----------------------------------------------------------- This patch is now in conflict with the patch in review #63. Given that #63 has urgency, can we please land that one and rebase this patch atop it? - Stephen Gallagher On Aug. 20, 2014, 6:59 p.m., Thomas Woerner wrote:

----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard-fedoraserver.rhcloud.com/r/17/ ----------------------------------------------------------- (Updated Aug. 22, 2014, 7:36 p.m.) Review request for RoleKit Mailing List, Stephen Gallagher and Thomas Woerner. Repository: rolekit Description ------- Simplifies the role interface Role cleanup by dropping "failonthis" setting Diffs (updated) ----- config/roles/databaseserver/role.py 4811f787ae4fe79b84e921b8c6a9cd65acc1d792 config/roles/domaincontroller/role.py 8f02216edd52d40761152a2238171ebaf9abf253 config/roles/testrole/role.py 00fa54f52530789c640f7b2adde8f333a0eebf7a src/rolekit/server/rolebase.py 8a618a59ee76c602504d0bfa758ca337aefce7b2 Diff: http://reviewboard-fedoraserver.rhcloud.com/r/17/diff/ Testing ------- Thanks, Thomas Woerner

----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard-fedoraserver.rhcloud.com/r/17/#review267 ----------------------------------------------------------- Ship it! Ship It! - Stephen Gallagher On Aug. 22, 2014, 7:40 p.m., Thomas Woerner wrote:

----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard-fedoraserver.rhcloud.com/r/17/ ----------------------------------------------------------- (Updated Aug. 29, 2014, 12:33 p.m.) Status ------ This change has been marked as submitted. Review request for RoleKit Mailing List, Stephen Gallagher and Thomas Woerner. Repository: rolekit Description ------- Simplifies the role interface Role cleanup by dropping "failonthis" setting Diffs ----- config/roles/databaseserver/role.py 4811f787ae4fe79b84e921b8c6a9cd65acc1d792 config/roles/domaincontroller/role.py 8f02216edd52d40761152a2238171ebaf9abf253 config/roles/testrole/role.py 00fa54f52530789c640f7b2adde8f333a0eebf7a src/rolekit/server/rolebase.py 8a618a59ee76c602504d0bfa758ca337aefce7b2 Diff: http://reviewboard-fedoraserver.rhcloud.com/r/17/diff/ Testing ------- Thanks, Thomas Woerner