Page MenuHomeFreeBSD

Update review requests for bhyve.
ClosedPublic

Authored by jhb on Nov 5 2018, 5:54 PM.

Details

Summary
  • Explicitly mention the bhyve group on Phabricator.
  • Request reviews of the userland components (libvmmapi, bhyve, bhyvectl, and bhyveload).

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

jhb created this revision.Nov 5 2018, 5:54 PM
araujo accepted this revision.Nov 5 2018, 6:01 PM
araujo added a subscriber: araujo.

Sounds reasonable!

This revision is now accepted and ready to land.Nov 5 2018, 6:01 PM
imp accepted this revision.Nov 5 2018, 6:07 PM

Seems to achieve the desired policy goals.

rgrimes added a subscriber: rgrimes.Nov 5 2018, 6:22 PM

This is good, but it is not clear on *who* can approve as bhyve, in the past that was Peter, is it now jhb & tychon, or anyone who just happens to join the bhyve group, which is not controller.

rgrimes accepted this revision.Nov 5 2018, 6:23 PM
araujo added a comment.Nov 5 2018, 6:26 PM

This is good, but it is not clear on *who* can approve as bhyve, in the past that was Peter, is it now jhb & tychon, or anyone who just happens to join the bhyve group, which is not controller.

If the bhyve group cannot be controlled, in this case would be better add the team members name explicitly.
I thought the bhyve group were controlled.

This is good, but it is not clear on *who* can approve as bhyve, in the past that was Peter, is it now jhb & tychon, or anyone who just happens to join the bhyve group, which is not controller.

If the bhyve group cannot be controlled, in this case would be better add the team members name explicitly.
I thought the bhyve group were controlled.

It is controlled to the extent that only a committer can join it, but any committer who has joined it well get the bhyve umbrella accept check by default if they are a member and do an accept of a bhyve review. This could be changed. It is just how allanjude set it up when it was created.

araujo requested changes to this revision.Nov 6 2018, 3:53 AM

After reflect a little bit more about what @rgrimes mentioned, how about we do something like this?

usr.sbin/bhyve* tychon, jhb, anish, araujo, rgrimes <...> Pre-commit review requested via #bhyve phabricator group.

It would applies also to libvmmapi and friends.

This revision now requires changes to proceed.Nov 6 2018, 3:53 AM
jhb added a comment.Feb 7 2019, 6:29 PM

I think listing a lot of folks will start to get unreadable. I think we can start with the existing language and just use some common sense (if a couple of folks who aren't random committers approve userland changes, then that's probably fine). If we have to nail it down further in the future we can revisit it then.

rgrimes accepted this revision.Feb 7 2019, 7:14 PM

I am good with the language as it is for now, lets get this commited sooner rather than later please.

araujo accepted this revision.Feb 8 2019, 10:06 PM

I still feel locked out of usr.sbin/bhyve, mostly of the commits made there in the past year were made by myself or reviewed by myself. Would be fair have my name explicit in that area, I don't want bypass reviews, it is more about the effort I have put in that area and appreciation of it, otherwise doesn't make sense for me to keep contributing with bhyve.

I'm approving with disapproval.

This revision is now accepted and ready to land.Feb 8 2019, 10:06 PM
imp added a comment.Feb 9 2019, 5:43 AM

Would it make sense to add araujo's name to the usr.sbin/byhve line?

araujo removed a reviewer: araujo.Feb 22 2019, 1:23 AM
araujo removed a subscriber: araujo.
This revision was automatically updated to reflect the committed changes.