RFC: Commit access for outside contributors

[octo]

CC: @rubenk, @rpv-tomsk, @tokkee

Hi team,

as you may be aware, the Automerge label will cause @collectd-bot to automatically merge pull requests when certain conditions are met (CI succeeds, no pending reviews, …). I'm thinking about changing the behavior a bit and would like your thoughts on this:

1. Require one "approved" review from one of the maintainers instead of the Automerge label. I.e. as soon as a maintainer approves a change, it will automatically get merged (assuming CI is successful).
2. As a second step, I'd like to integrate with Github's code ownership concept. When, for example, a pull request only touches src/dpdk{events,stat}.c, then a (positive) review from one of the Intel folks working on this should be sufficient to trigger the merge. We should encourage contributors to add themselves as owners of new plugins.

Caveats:

• You can now "resolve" code review comments in the UI, but as far as I see it's not possible to distinguish "resolved" from "unresolved" comments in the API. That means you'd have to ensure all comments are fixed before approving.
• It's unclear how much of the code ownership we have to implement ourselves. I'm hoping that with the "Require review from Code Owner" option enabled a PR is simply marked as "not mergable" unless the condition is met. But the API docs are extremely vague with details like this.

What do you think?

Best regards,
—octo

[rpv-tomsk]

Hi!

Require one "approved" review from one of the maintainers instead of the Automerge label. I.e. as soon as a maintainer approves a change, it will automatically get merged (assuming CI is successful).

As for me, I used "Automerge" label when I have some small change to commit, but I want PR to pass CI checks. So, I make PR, give it "Automerge" label and bot cares about PR, not me.

Next time, if I see what CI has failed, I will resolve that issue, or take no more care if CI succeeds.
(Ok, I also check, if CI of master has not failed after merge of succeeded PR check, that can happen, unfortunately.)

Require one "approved" review from one of the maintainers instead of the Automerge label. I.e. as soon as a maintainer approves a change, it will automatically get merged (assuming CI is successful).

At practice, there was cases when PR has approved maintainer review, but was not merged by anybody.
Proposed change will cover such cases, but most of the time, I think, when maintainer reviews PR, it already has CI checks completed, so he can just do merge directly. If maintainer doesn't want to take responsibility of merge, then he will not approve PR at all after such change.

Also, this mades impossible to require review from two (or more) maintainers, when one checks code style and other can check implementation details. Should we give it up?

---

I'm hoping that with the "Require review from Code Owner" option enabled a PR is simply marked as "not mergable" unless the condition is met.

With protected branches enabled, a code owner for each owned file has to leave a review before anyone can merge a pull request to that branch.

Will this change make it impossible to merge PR by maintainers w/o Code Owner Review? This unclear for me a bit.

"Code Owner Review" implementation can make things faster to merge, which is attractive for committers.
But we also need to have more transparent release cycle and list of supported versions.

[octo]

Hi Pavel!

Thanks for your feedback. To clarify, my goal is to allow contributors to maintain "their" plugin without being blocked by us maintainers. Maybe we can achieve this with Github's owner concept alone, i.e. without involving the bot.

For example, consider this config:

# These owners will be the default owners for everything in the repo.
*       @collectd/maintainers

# Docs
/README @collectd/everybody
*.pod   @collectd/everybody

/src/example.c  @individual-author
/src/dpdkstat.c @contributor1 @contributor2

We could then add "individual-author" as an outside contributor. This user should then be able to approve (and merge) changes to src/example.c. Changes by "individual-author" themselves should still require a review by one of the maintainers though.

The other example, src/dpdkstat.c, would allow "contributor1" and "contributor2" to work together to maintain that code – without any involvement of the core maintainers.

[rpv-tomsk]

So, if example.c has two Code Owners, then one can put PR and only second owner can approve it? Sounds good.

With protected branches enabled, a code owner for each owned file has to leave a review before anyone can merge a pull request to that branch.

I just don’t want to catch a situation when maintainer will not be able to merge commit w/o code owner.

[octo]

The intention is that changes to example.c require approval from the plugin owner(s) or a "core maintainer" (the GitHub docs call these "global owners").

[rpv-tomsk]

Сounterexample: PR #2874. It has two approves from code owners and change request from me.

[octo]

My expectation would be for Github to block the commit until you are happy and change the status of your review to APPROVED. In other words, for 2874 your review should not be required, but if you chose to do a review merging should be blocked until you too approve the changes.

[rpv-tomsk]

... Then team of committers can do /relatively/ fast approve, so I'll be late ))
Maybe there is 'grace period' exits ? )

[octo]

team of committers can do /relatively/ fast approve

That's precisely my goal 😁 I'd like the "core maintainers to be less of a bottle neck and allow outside contributors to move fast, without giving away the keys to the castle, so to speak.

[rpv-tomsk]

I am afraid that speed will be at the price of quality.

I propose to analyze a couple of examples from the latest merges.

[octo]

I absolutely understand that concern – I had the same concern every time I gave commit access to someone. My experience has been, though, that if you trust people they will repay you by behaving responsibly. That said, if it doesn't work out and we come to the conclusion that revoking commit access from an outside contributors will reduce work for maintainers, we can (and should) do that.

[octo]

FYI, we're testing this with a couple of people from Intel. If this works well, we'll try to get this established more broadly.

[rpv-tomsk]

Hi, Team.

I don't know, is this issue is correct target for this write, but I found no other which suits better.

This week I found some time to continue work on Collectd project.

Last months I read all the messages in mailing lists and from Github, so I know that changes are in progress.

Today I noticed the fact, which is very improtant for me and for project: I see, maintainers can't merge theirs PR yourself.

I know, that reviews are good to check project policy, code quality, etc.

But by previous practice, I know what PR may remain without review even for years.
So, I think, this strict rule will stop (at least my) development, and I see signs of this right now.

After I became project maintainer, I made many PR and many commits. Many of them was merged by me directly.
I think, nobody from Team and Community complaints my work.

But today, when new strict rules are here, I'm not ready to write PR to put them under the cloth and
wait for 'somebody' to pay attention, like it was some years before.

This is all I wanted to draw Team and Communty attention.
Thanks.

[octo]

Hi Pavel,

I think this is the right place for this conversation.

The implementation of per-plugin owners currently requires that we enable the "Require review from Code Owners" feature to make it work. We can only grant "write" permissions (i.e. commit access) to the repository as a whole. We then restrict what per-plugin owners can commit by requiring reviews from a "code owner".

This change is purely a side-effect and not the result of any misbehavior. I'm very happy with the work that you and all the other maintainers are doing.

Please keep in mind that this is an experiment. If, after a while (I'd say 3–6 months) we conclude that this is a net negative for the project, we'll modify or stop the experiment.

Best regards,
—octo

[sunkuranganath]

Hi Octo,

Agree with your comment on maintainers. You have been helpful.

One question on the experiment part. We as read plugin developers or code owners (for now), welcome this direction.
From your perspective, what would you categorize as a failure of the experiment?
For now "Code Owners" only own/control specific set of read plugins. Do you see there might be a net negative effect to the core of the daemon?

Thanks,
Sunku

[rpv-tomsk]

As for me, there is none of positive changes.
There is no new commits authors. Latest commits and merges was done by same people as before these changes.

Also, I see negative changes against me (personally).
Example: #3116

I made this PR 3 weeks ago. Last update was 1 week ago (just to restart failed CI check).
Before these changes I was able to do this merge myself.

After changes, I have no such possibility, and I have to wait until the thunder claps.
As I remember, that waiting can last for months.

[sunkuranganath]

@rpv-tomsk sorry to hear about your experience.
Out of the plugins that were provided with committer access to our team, RDT, Virt, PMU and RedFish, our team has submitted multiple changes in RDT and Virt plugins, since updating them has been our priority for the 5.9 release to start with.
For RDT #2891 , we value your review and since you have been looking at it even before commit access, neither @kwiatrox nor myself went ahead with merge ourselves. We were looking to get a yes from you.

For Virt #3117 similar story. One of the commit author @rjablonx has contributed multiple updates. Both @rubenk and yourself have been providing valuable feedback. So want to ensure we go with you on it.

For virt #3116 one of the commit owner @anaudx has provided the review and approved the changes with in 2 weeks of having this PR. If you see there is a delay from our side please flag it and we will ensure we give high priority. I believe @anaudx is good to have this merged, but will let him comment and merge it.

We have seen similar challenge of waiting for "thunder claps" like you mentioned when there are very many issues/PRs to be accepted by maintainers. Our goal and hope, as commit owners, is to reduce the burden by helping accelerate the reviews in the community without side tracking the maintainers. Until we clear out a easy way of working together, this might seem some inconvenience. But we are definitely open to help and learn.

As per new PRs, we have few plugins in the works and going through internal reviews/approvals. So you would see more updates from us soon.

Let us know how we can make this easy for you.

Thanks
-Sunku

[rpv-tomsk]

If you see there is a delay from our side please flag it and we will ensure we give high priority.

"flag it" ?? ouch. I see nobody have responsibility.

For virt #3116 one of the commit owner @anaudx has provided the review and approved the changes with in 2 weeks of having this PR.

I see this PR as not merged. That is all I see.

I can't do anything about it, because now I have no merge rights for my own changes.
And nobody cares.

[rpv-tomsk]

RFC: Commit access for outside contributors

Please rename this issue to "Remove commit access for "inside contributors" ". ? :)

[anaudx]

@rpv-tomsk I approved virt PR #3116 on 13-03-2019 (and second time a while ago) and AFAIK this should automatically merge the PR, but as we can see this does not happen. I don't have any other possibility to merge other than by approving the PR.

[rpv-tomsk]

Thanks for your try ;-)

[sunkuranganath]

RFC: Commit access for outside contributors

Please rename this issue to "Remove commit access for "inside contributors" ". ? :)

Looks like the issue that you have trouble with is about merging any of your updates.

Do you have any specific suggestions to help both code owners and maintainers? :)

[anaudx]

What is expected from me as codeowner in this scenario?

[rpv-tomsk]

As Commit access for outside contributors feature removes commit access for 'inside contributors', should I create another Github account to become 'outside contributor' too, to get possibility to merge my PR myself?

Thanks.

[sunkuranganath]

Guess we all are waiting for @octo to respond!

[rpv-tomsk]

PR #3148 hang in status "ChangeLog Expected — Waiting for status to be reported".
PR #3149 come to status failed "continuous-integration/travis-ci/pr — The Travis CI build failed", where one of the three jobs failed with message "checking whether build environment is sane... configure: error: newly created file is older than distributed files!" and other two are ok. Later, the same happened with #3156.

Master branch CI checks failing for months due to failing Solaris checks (#2815).

As Developer and Maintainer, I'm not going to fight these systems and newly-added restrictions.

@octo, you really need to solve these CI problems.

Also, https://pkg.ci.collectd.org has no actual distributions supported, last is Debian Jessie, for example.
And there is no actual Collectd version, latest is 5.8.0 but we have 5.8.1 released (#2992).

If you really want to bury The Project (thanks for founding it), please declare that explicitly.

Thanks.

[rpv-tomsk]

Please keep in mind that this is an experiment. If, after a while (I'd say 3–6 months) we conclude that this is a net negative for the project, we'll modify or stop the experiment.

I see no benefits from this experiment. I see no PR merged by "outside contributors".
Also, I vote against this because code quality will worsen.

I'm not able to understand, why you wait for 3 months to restore access for "inside contributors".

I'm not able to work on Project while all actions done are against this my work.
That is contr-productive.

[rpv-tomsk]

Another example: #3127.

When I had access, I could simply move one line of code into appropriate place.
That takes me 5 minutes to solve problem.

Now, I should watch into opened PR and wait until someone’s attention, which possibly never happen.
So, I simply prefer not to spent my time.
Did you achieve this intentionally?

[rpv-tomsk]

Merge is blocked for #3163 even with approved review from project member.

[usev6]

At the risk of a) annoying people and b) coming across as a know-all: From the perspective of an outsider this workflow change (and probably breakage) looks like a serious problem. One of the more active developers pointed out that his contributions are hampered by the current configuration and expressed his concerns loudly. If there was a 'blocker' label, this issue would deserve it IMHO.

[mrunge]

I always understood this change as additional possibility to get something merged, not as intention to remove people.

The issue mentioned here with "code-owners" approving a patch: that was apparently due to "code-owners missing write permissions in the repository and should be fixed.

Unfortunately, I don't have an insight, why approval of a trusted member does not trigger a merge through the automerge bot.

Anyways: the last thing we'd want is either to discourage or actively prevent people from contributing. If you feel that way, please speak out!

@rpv-tomsk Pavel, I've heard your concern, and let's see what we can do to fix it. Please bear with us.

[rpv-tomsk]

I always understood this change as additional possibility to get something merged, not as intention to remove people.

There is declarations on words, and practice at reality.
They mismatch.
I am deprived of the right to do commits for more than 4 months.

As you can see, my question and "side effect" left w/o answer/fix from 21 Feb.

How do you think, how this affects my desire to continue work on project?

[mrunge]

How do you think, how this affects my desire to continue work on project?

If this is not meant to be a rhetorical question, I assume you're demotivated and feel neglected. As said: let's try to get this fixed ASAP.

Meanwhile, please feel free to add me to your PRs, and I'm happy to review/merge them.

[rpv-tomsk]

As per https://github.com/orgs/collectd/teams/trusted-contributors/members there are 8 members,
including me and you.

Let's look, how many comments/reviews I got in #3159 where I asked review from "Trusted Contributors"?
(Ok, that change is serious enough. I will even not merge it myself without review ever I had such permission, as it was before "provide write access to outside contributors" removed it. But there can be other examples too.)

So, you propose me to write PR and wait. Wait. Wait and wait again. For months.
I already went through it.

---

UPD: Thanks for your care on my PRs.

[rpv-tomsk]

As you all can see, hiding behind good intentions, "TEAM" found new way to break Collectd development.

Not only administrative rules applied now, but technical too.

New "clang-format" rules prohibits to merge PR, even if no lines was added - see: #3214
That file was passed through contrib/format.sh many times before, so it should pass.

To overcome this barrier, developer needs to run stupid contrib/format.sh.
In my case there was only 3 line removed, but after that tool run file has 20 lines changed.

You think your PR is ready, but it is not. That additional stupid work makes developers angry and not loyal to project.

That makes PR hard to review, so PR merge stops and develoment stops as result.

I see only these, one after another changes appear which breaks development flow and destroy the project.

[rpv-tomsk]

GoodBye, team, I don't see what I can do here.

I leave project's "Trusted Contributors" team, that has no sense for me anymore.

FREEDOM!

[mrunge]

I don't think this outcome is/was intended/desired at all. During working with the changed rules/setup we discovered the following issues:

• for some reason, clang-format requires files to be re-formatted, even if they were not changed
• When doing large changes, such as #3153 , not only trusted contributors, but also "code owners" need to approve a patch.

[sunkuranganath]

Agree with @mrunge. The procedures being put in place should make it easy for developers and maintainers alike. Trying to reach out to @octo in various ways so we make this easy going ahead.

[rpv-tomsk]

@sunkuranganath commented in #3072

We all work on this in our own time on top of our other responsibilities and want to make this work.

I'm speaking about the same - our's time is the most valuable resource, so we should not spent it on fighting administrative and CI issues. Also, I think, nobody of us wants to spent time implementing feature and then throw out the result of the work just because lack of response. Yes, I know that is the problem 'code owners' tries to solve (add plugin owners ability to merge theirs changes), but that breaks things and puts them upside down.

Just looked into my PR history.

For example, PR 2819.
There was lot of work, done for project.
I don't use SNMP seriously, snmp plugin from 5.7 is enough for me to get stats from one switch I have.
But that work was done. And that takes many hours.
What do you think, who from "trusted contributors" reviewed that code?
What do you think, when that PR would be merged, if I don't merge it myself? I don't want to check this.

There was 4 days w/o any feedback, and I found this not funny to spent hours of work, so just do that merge myself.

Because I could.

When I could't:

(just some stats, I don't looked over all 135 my PR and don't included PRs where waiting was month or less).

Fresh example:
3159 - check_uptime: New plugin, based on new cache_event callback.

opened on 19 May 2019. No any feedback for ~2months.

Historical examples:

---

2391 - perl plugin: Added check of proper interpreter initialization.

~2 months of waiting, opened 2 Aug 2017 - merged 21 Sep 2017;

---

2388 - memcached: Persistent connections with IO timeouts

2 months of waiting, opened 31 Jul 2017 - merged 27 Sep 2017;

---

2385 - memcached: Fix hitratio reporting, add connections rate report

opened 30 Jul 2017, first review 26 Sep 2017

---

2377 - openldap: Fix crash on shutdown when openldap connection failed

2 months of waiting, opened 24 Jul 2017 - merged 22 Sep 2017

---

2232 - processes: Show real disk IO in addition to process IO (Linux only)

4 months of waiting, opened 29 Mar 2017, first review - 17 May 2017, merged 20 Jul 2017

---

2024 - ipmi plugin: Implemented IPMI LAN connection and plugin improved

11 months of waiting, opened 5 Nov 2016. First review 8 Oct 2017. Merged 10 Oct 2017.

I do that merge myself, that was my first merge as maintainer.

---

1622 - statsd, utils_latency: allow to reduce bin width

5 months of waiting, opened 20 Mar 2016 - merged 11 Aug 2016. 2.5 months before first review plus two months after, before PR merged.

---

1564 - disk plugin: send disk_merged,disk_io_time,pending_operations only when real data exists for device
1565 - disk plugin: call ignorelist_match() once per device, not for each send metric

2 months of waiting, opened 25 Feb 2016 - merged 24 Apr 2016;

---

1558 - Curl_xml: Custom names for plugin/plugin instances

7 months of waiting, opened 24 Feb 2016 - firstly reviewed and merged shortly after 29 Sep 2017;

---

Who of you likes to wait?