Maintainers update

Development
1

Hello,

there was another meeting of Foreman core maintainers. Last time we kicked off the mentorship program, more to be found here. Today we discussed how we improve the PR review process itself.

We’d like to officially separate code review from testing. It does not change today practise, but everyone should be aware that only reviewing the code already helps. The same applies for testing. The lifecycle of the PR should start with the code review and only after the code is acked, testing should start, to avoid unnecessary retesting. We’d like to improve the tooling, gh labels etc around this, but we didn’t discuss details and possibilities (wink wink @ekohl), let’s try it here. Few things raised - ansible repo has nice automation we can inspire from, setting up testing env with the PR applied may help, e.g. using the container we built with every commit.

One thing we know though is, we’ll start using PR assignee. Until the PR is assigned, no reviewer officially works on the review. The first thing we do when we start reviewing is, assigning ourselves to the PR. This should help with situations, when it’s not clear who owns it.

So the flow should look like this (quick sketch, probably not capturing each and every possibility)

pr
pr721×381 25 KB

We’d be happy to hear feedback on this and how to support the process with tooling in a way it’s clear to contributors and everyone know what is the current status.

The mantainers will meet on biweekly basis from now and assign reviewer to PRs based on current capacity. We want to start with the oldest ones. In that 2 week window, we should either decide to close or do the code review.

We also discussed the revoking permissions of inactive maintainers. While we don’t want to revoke permissions, we would like to share our concern with longer inactivity. Maintainers who are not involved for more than 6 months with the project may have lost overview, so they should proceed more carefully, if they decide to merge something. So after 6 months of inactivity we may reach out and see if the maintainer still want to be somehow involved, to set the right expectations on both sides. Activity in this case does not necessarily mean PR reviews, but contribution to the project in general. People can always give up permissions voluntarily.

Maintainers who were in that meeting, please add anything I missed.

2 Likes
2

I appreciate the fact you are committed to the process of reviewing and merging PRs. +1

The only thing that was missing for me is how it is going to affect me as a reviewer who is not a maintainer.

Before, I could open whatever PR I see in the list and review it if/when/how I see it right.
Now I should stop/change my behavior? Will it lead to me do fewer reviews?

3

Often I want to do some drive by remarks. I can’t fully commit to the entire review because I don’t know enough about Ruby/Rails. However, often I can point out things that wouldn’t work in the broader ecosystem or things to be aware of. If I need to fully commit there’s a good chance I’ll stop the partial reviews. However, I also see how it can be confusing that someone with merge access does a review but won’t merge it. How do you see this? This is basically the same problem as @sharvit has but from another angle.

4

Thanks for your question Avi. Last thing we want to do is discouraging code reviews :slight_smile:

If you want to commit yourself for the full code review, please do the review as usual, if you can’t assign yourself to the PR due to permissions (need to check), add a comment like “I’ll be the reviewer”. Someone can assign you. This would mean we rely on you to do the code review. Once the code is also tested, some maintainer will do the merge if all is OK.

I’ll address the other option of doing just a partial review as a reply to Ewoud.

1 Like
5

For “partial” reviews, nothing really changes. What should be formalized though is, whether your comments are blocking or not. If you require your comments to block the merging, then use “require changes”, if you suggest things for considerations, use “comment only” reviews. Extra points for being explicit in the comment, so that contributor knows, this is not the full review and the PR is still in the queue. E.g. “please don’t consider this a full review but I’m adding few suggestions”.

That does not change the fact, we need to find someone, who commits for the full review, in order to get the PR on mergeable path.

I hope that answers your question. i’d be happy to hear your thoughts.

Also any other suggestions from anyone really are welcome.

1 Like
6

Sorry for not being able to attend the meeting. I think the ideas that came up are great. Looking forward to seeing this in action.

7

The idea was actually unblock you from feeling responsible to come back if you just left some notes. Today it’s often considered blocking. So introducing the assigned reviewer and everyone to know "If you want to block, use “request changes” should actually cause the oposite of what you are concerned about :slight_smile:

Exactly like @Marek_Hulan put it.

8

I would love to formalized this - in a similar way of the graph Marek posted - in the developer docs though. Any ideas how to do being as easy to maintain as possible?