Once reviewers think your code is ready, you can […] send RTI mail. Depends on the situation, you may also need to further polish your code.
Huh, that’s basically two levels of review, and the second one is using mail instead of a proper tool that the first one already uses.
In FreeBSD and Mozilla, committers pick stuff up from Bugzilla&Phabricator directly, which is very convenient. Why does illumos have such a convoluted process?
First up, we’re definitely moving away from mail-based RTI submission this year. I and most other people agree the mail stuff is tedious, and is at this point primarily just a long shadow of a process we started with about a decade ago, based on even earlier processes before that. Indeed, it is acceptable to just link to your existing Gerrit review today, rather than attaching a patch, which is what most regulars are doing. I’ll go and amend the contribution guide to reflect that as the easier option.
Second, though I just wanted to clarify that we’re not really doing two code review passes here. The integration approval step is chiefly about risk management and ensuring that each change meets a few basics that aren’t really a part of code review per se; i.e., that the change definitely doesn’t break the build, that you’ve been able to demonstrate that it has been tested appropriately or that such testing would be impossible due to limited resources, etc. We try to keep the code review phase mostly about looking through the code change for bugs, which allows relative newcomers to get involved as reviewers without needing a deep understanding of what testing would be appropriate, etc.
In general for a small change like this that has seen code review I would expect integration without any back and forth. In this specific case, I believe the contributor was at least relatively new and did not fully appreciate that the lint warnings in a manual page are something we want people to fix as they make their change. We asked them to do so, and they did. I’m sure this is also at least in part a documentation issue on our end, and I’ll see if I can improve things there.
The process was definitely more convoluted in the past, and I’m looking to continue streamlining it this year, while retaining the good parts like explicitly documenting testing, etc.
Huh, that’s basically two levels of review, and the second one is using mail instead of a proper tool that the first one already uses.
In FreeBSD and Mozilla, committers pick stuff up from Bugzilla&Phabricator directly, which is very convenient. Why does illumos have such a convoluted process?
First up, we’re definitely moving away from mail-based RTI submission this year. I and most other people agree the mail stuff is tedious, and is at this point primarily just a long shadow of a process we started with about a decade ago, based on even earlier processes before that. Indeed, it is acceptable to just link to your existing Gerrit review today, rather than attaching a patch, which is what most regulars are doing. I’ll go and amend the contribution guide to reflect that as the easier option.
Second, though I just wanted to clarify that we’re not really doing two code review passes here. The integration approval step is chiefly about risk management and ensuring that each change meets a few basics that aren’t really a part of code review per se; i.e., that the change definitely doesn’t break the build, that you’ve been able to demonstrate that it has been tested appropriately or that such testing would be impossible due to limited resources, etc. We try to keep the code review phase mostly about looking through the code change for bugs, which allows relative newcomers to get involved as reviewers without needing a deep understanding of what testing would be appropriate, etc.
In general for a small change like this that has seen code review I would expect integration without any back and forth. In this specific case, I believe the contributor was at least relatively new and did not fully appreciate that the lint warnings in a manual page are something we want people to fix as they make their change. We asked them to do so, and they did. I’m sure this is also at least in part a documentation issue on our end, and I’ll see if I can improve things there.
The process was definitely more convoluted in the past, and I’m looking to continue streamlining it this year, while retaining the good parts like explicitly documenting testing, etc.