On Aug. 19, 2014, 2:56 p.m., Miloslav Trmac wrote:

...

config/roles/testrole/role.py, lines 105-106 http://reviewboard-fedoraserver.rhcloud.com/r/66/diff/1/?file=259#file259line105

Documentation IMHO belongs into RoleBase, not to be copy&pasted in all roles.  If necessary, attach it to to a method that only raises NotImplemeted (or returns False?)

Also, please keep the interfaces of all roles in sync (i.e. when adding a method to one, add it to all.)

Miloslav Trmac wrote: And finally, having this a docstring instead of a comment would be nice. Tools understand docstrings.

I think that a writer of a role should have an example role, that is explaining all the things he needs to add and also in which way. If a role writer has to look in RoleBase he will find a lot of stuff that might not be of interest in creating a role. But RoleBase might be good as a reference if there are open questions.

The role itself does not necessarily care a lot about D-Bus stuff.

- Thomas

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

On Aug. 18, 2014, 12:03 p.m., Thomas Woerner wrote:

---

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

---

(Updated Aug. 18, 2014, 12:03 p.m.)

Review request for RoleKit Mailing List, Miloslav Trmac, Stephen Gallagher, and Thomas Woerner.

Repository: rolekit

Description

This is needed to be able to check types of role specific properties.

Diffs

config/roles/testrole/role.py 358deca3fc7172929d53d2c77efd5c919da2aea9 src/rolekit/server/rolebase.py 8591f8aade76d3463647c59b43cc83877698182d

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

Testing

Thanks,

Thomas Woerner

On Srp. 19, 2014, 4:56 odp., Miloslav Trmac wrote:

...

config/roles/testrole/role.py, lines 105-106 http://reviewboard-fedoraserver.rhcloud.com/r/66/diff/1/?file=259#file259line105

Documentation IMHO belongs into RoleBase, not to be copy&pasted in all roles.  If necessary, attach it to to a method that only raises NotImplemeted (or returns False?)

Also, please keep the interfaces of all roles in sync (i.e. when adding a method to one, add it to all.)

Miloslav Trmac wrote: And finally, having this a docstring instead of a comment would be nice. Tools understand docstrings.

Thomas Woerner wrote: I think that a writer of a role should have an example role, that is explaining all the things he needs to add and also in which way. If a role writer has to look in RoleBase he will find a lot of stuff that might not be of interest in creating a role. But RoleBase might be good as a reference if there are open questions.

The role itself does not necessarily care a lot about D-Bus stuff.

Miloslav Trmac wrote: IMHO the _API_ should have a documentation in exactly a single place, and that’s a docstring within RoleBase (perhaps in an “abstract method”).

Examples and the like should ideally not be necessary beyond the API documentation, but even if so, please keep the other roles in sync at least WRT the non-comment lines (we are “writers" of those roles).  Both me and Stephan have already ended up copy&pasting testrole into the other roles because they were not in sync; this was feasible because the other roles had no content, but now that they do, having to manually review the changelog of testrole and see which commits to mirror in the other roles is unnecessary work (and one wouldn’t even notice that the work is necessary, the roles will just break if not updated).

Oops, now I have noticed that the updated patch already keeps the roles in sync.

- Miloslav

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

On Srp. 20, 2014, 8:02 odp., Thomas Woerner wrote:

---

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

---

(Updated Srp. 20, 2014, 8:02 odp.)

Review request for RoleKit Mailing List, Miloslav Trmac, Stephen Gallagher, and Thomas Woerner.

Repository: rolekit

Description

This is needed to be able to check types of role specific properties.

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

Testing

Thanks,

Thomas Woerner

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

(Updated Sept. 2, 2014, 2:57 p.m.)

Review request for RoleKit Mailing List, Miloslav Trmac, Stephen Gallagher, and Thomas Woerner.

Repository: rolekit

Description -------

This is needed to be able to check types of role specific properties.

Diffs (updated) -----

config/roles/databaseserver/role.py 0439a56c7617a947da8f37fc3493751667a24feb config/roles/domaincontroller/role.py 774b2ad45fd3fea2f94c58cd20094f6a61acbdef config/roles/testrole/role.py 8d7499af5df3317cbf3fc2671e59f36acd3b1982 src/rolekit/server/rolebase.py 95b10c62ff44fbf12d96992bb07fc68d156259ca

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

Testing -------

Thanks,

Thomas Woerner

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

Ship it!

Ship It!

- Stephen Gallagher

On Oct. 2, 2014, 2:18 p.m., Thomas Woerner wrote:

---

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

---

(Updated Oct. 2, 2014, 2:18 p.m.)

Review request for RoleKit Mailing List, Miloslav Trmac, Stephen Gallagher, and Thomas Woerner.

Repository: rolekit

Description

This is needed to be able to check types of role specific properties.

This patch adds the framework to be able to check role specific properties automatically with the do_check_property method if they are set for example in a deploy call.

Diffs

config/roles/databaseserver/role.py 0439a56c7617a947da8f37fc3493751667a24feb config/roles/domaincontroller/role.py 774b2ad45fd3fea2f94c58cd20094f6a61acbdef config/roles/testrole/role.py 8d7499af5df3317cbf3fc2671e59f36acd3b1982 src/rolekit/server/rolebase.py 95b10c62ff44fbf12d96992bb07fc68d156259ca

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

Testing

Thanks,

Thomas Woerner