On Čec. 21, 2014, 8:33 odp., Miloslav Trmac wrote:

...

src/rolekit/server/rolebase.py, line 165 http://reviewboard-fedoraserver.rhcloud.com/r/17/diff/1/?file=92#file92line165

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

(Or is there a guarantee that None can never be a valid property value? If so, None could be used instead of PropertyNotRecognizedValue.)

On Čec. 21, 2014, 8:33 odp., Miloslav Trmac wrote:

...

src/rolekit/server/rolebase.py, line 194 http://reviewboard-fedoraserver.rhcloud.com/r/17/diff/1/?file=92#file92line194

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

(Or is there a guarantee that None can never be a valid property value? If so, None could be used instead of PropertyNotRecognizedValue.)

- Miloslav

----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard-fedoraserver.rhcloud.com/r/17/#review57 -----------------------------------------------------------

On Čec. 21, 2014, 3:16 odp., Thomas Woerner wrote:

---

This is an automatically generated e-mail. To reply, visit: http://reviewboard-fedoraserver.rhcloud.com/r/17/

---

(Updated Čec. 21, 2014, 3:16 odp.)

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

On July 21, 2014, 6:33 p.m., Miloslav Trmac wrote:

...

config/roles/testrole/role.py, lines 109-116 http://reviewboard-fedoraserver.rhcloud.com/r/17/diff/1/?file=91#file91line109

Silently returns None on an unmatched property

Thomas Woerner wrote: get_property is not needed if the setting is in _DEFAULTS, see RoleBase.get_property:

        if hasattr(x, "_settings") and prop in x._settings:
            return x._settings[prop]
        ...
        elif prop in x._DEFAULTS:
            return x._DEFAULTS[prop]

Therefore settings in _settings and in _DEFAULTS are already taken care of. This means get_property should not be needed in a Role.

Miloslav Trmac wrote: The concern is about completely unknown setting names; the final do_get_property() just returns None for them.

Dropped do_get_property from roles completely.

- Thomas

----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard-fedoraserver.rhcloud.com/r/17/#review57 -----------------------------------------------------------

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/

---

(Updated July 22, 2014, 3:35 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

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/ -----------------------------------------------------------

(Updated July 22, 2014, 3:35 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 -----

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/#review199 -----------------------------------------------------------

config/roles/testrole/role.py http://reviewboard-fedoraserver.rhcloud.com/r/17/#comment113

shouldn't you change get_property -> do_get_property ?

config/roles/testrole/role.py http://reviewboard-fedoraserver.rhcloud.com/r/17/#comment112

shouldn't you change the examples to do_get_dbus_property() too ?

config/roles/testrole/role.py http://reviewboard-fedoraserver.rhcloud.com/r/17/#comment111

is this improperly indented ? sounds like it should be in do_get_dbus_property() but it isn't this way

- Simo Sorce

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/

---

(Updated July 22, 2014, 3:35 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

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/ -----------------------------------------------------------

(Updated Aug. 20, 2014, 6:59 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 63687c0be4a5705e772f1a6d60932f69ce9850e7 config/roles/domaincontroller/role.py 358deca3fc7172929d53d2c77efd5c919da2aea9 config/roles/testrole/role.py 358deca3fc7172929d53d2c77efd5c919da2aea9 src/rolekit/server/rolebase.py cd28d6a47cf183f6017905034f178d7aecf83348

Diff: http://reviewboard-fedoraserver.rhcloud.com/r/17/diff/

Testing -------

Thanks,

Thomas Woerner

On Aug. 21, 2014, 1:21 a.m., Stephen Gallagher wrote:

...

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?

Yes, sure.

- Thomas

----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard-fedoraserver.rhcloud.com/r/17/#review255 -----------------------------------------------------------

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. 20, 2014, 6:59 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

config/roles/databaseserver/role.py 63687c0be4a5705e772f1a6d60932f69ce9850e7 config/roles/domaincontroller/role.py 358deca3fc7172929d53d2c77efd5c919da2aea9 config/roles/testrole/role.py 358deca3fc7172929d53d2c77efd5c919da2aea9 src/rolekit/server/rolebase.py cd28d6a47cf183f6017905034f178d7aecf83348

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/ -----------------------------------------------------------

(Updated Aug. 22, 2014, 7:40 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

On Aug. 22, 2014, 8:56 p.m., Stephen Gallagher wrote:

...

Ship It!

https://git.fedorahosted.org/cgit/rolekit.git/commit/?id=ca74ff1b687dba0eff5...

- Thomas

----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard-fedoraserver.rhcloud.com/r/17/#review267 -----------------------------------------------------------

On Aug. 29, 2014, 12:33 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.)

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