Skip to content
This repository has been archived by the owner on Nov 2, 2020. It is now read-only.

Commit bit pup #10

Closed
wants to merge 1 commit into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
101 changes: 101 additions & 0 deletions pup-0006.md
@@ -0,0 +1,101 @@
---
title: Commit Bit Assignment
author: Brian Bouterse
created: 01-06-2018
status: Approved
---

## Summary

For pulpcore and its related tools, there is no written process to describe giving the commit bit to
a contributor for all or a portion of the codebase. The existing committers want to openly document
the process.


## Motivation

Historically the commit bit was given on day 0 when hired on the core team at Red Hat. In Oct 2017
the existing committers decided to stop that practice and instead document an open process.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/committers/commit bit holders?

Engineers hired on the Pulp team since Oct '17 have not received commit bit.

We have not yet documented an open process of which to give a proven contributor commit access to
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/proven/trusted/

all or a portion of the codebase.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think it is important to stress that this process applies not only to RH engineers

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say the stress should be on the community



## Scope

This formally applies to the following core repositories and core-team maintained plugins:

Core Repos:
* pulp/pulp
* pulp/devel
* pulp/pulp-ci

Plugin Repos:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about future plugin repos which will be added and they are not on the list as of now?

* pulp/pulp_file
* pulp/pulp_python
* pulp/pulp_rpm
* pulp/crane
* pulp/pulp_docker
* pulp/pulp_puppet
* pulp/pulp_ostree

This policy is intended as the default for all software in the Pulp ecosystem unless explicitly
stated otherwise.


## Criteria

Overall the candidate must have demonstrated commitment and care towards the needs of all Pulp
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/candidate/applicant?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'd stick to candidate, are you still considered applicant in case someone else applied for you?

users and not only their own interests. Also they must have the experience to be trusted with major
aspects of Pulp functionality in that area.

These requirements are somewhat vague by design and leave the decision to the judgement of the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i do think that we need to write down some specific criteria.
from email thead ' i suggest we list at least some kind of criteria like X time involved in project development, has demonstrated to provide good form and useful code reviews, has submitted code that was accepted into main parts of the project.'

existing developers. Users rely on the judgement of developers already, so to rely on that judgement
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/existing developers/current commit bit owners/g (c ;)

to accept new contributors is reasonable. Anyone who specifically wants to get more involved should
approach the core devs about mentorship.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/core devs/current commit bit owners/

what's mentorship?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

guidance, on what needs to be done or fulfilled in order to be become eligible for commit bit acquisition.



## Process

A nomination or self-nomination can be sent to the pulp-dev email list and must contain state the
following:

* Name and email of the nominee. This can be a self-nomination.
* Repositories being requested
* [Specific paths](https://blog.github.com/2017-07-06-introducing-code-owners/) or the entire repo
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-1 on specific paths, i have a feeling it would introduce a lot of confusion.
say there is a PR for path A, B, C.
core reviewer has commit bit for A,C,D. he would review this PR but won't be able to merge.

Candidate applies for multiple paths commit bit acquisition, how the voting will go in case there will be different commit holders for different paths?

Copy link

@dparalen dparalen Jun 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

say we've got docs people, CI people and code people (in a single repository, for the respective paths), and say I've just sent a PR touching docs, CI and the code, how the PR going to be merged? Shall I go to every group and pull them for reviews?

How about wikis, issues etc?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect that some people will have ownership of more than one are of code. Some people may even have ownership of the entire repository. I would expect the docs to be owned by everyone who has a commit bit.

I doubt that the situation you are describing will be very common. In situations where a review is needed from 2 people in order to merge, we will have even more confidence that the code is introducing the intended changes.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry but it sounds like an unnecessary complication to me since I lack the knowledge of the motivation behind such a decision. If it indeed is the case that we objectively need the ability to selectively grant commit permissions to particular repository regions, we'd better clearly state reasons; although I can't prove it anyhow, this splitting doesn't seem a common practise in the communities.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not every Pulp commit bit holder possesses the knowledge of all the subsystems. e.g.:

@asmacdo has very deep knowledge of Pulp 3's viewsets and serializers while I only have some knowledge. I always ask @asmacdo to review changes in those areas.

@bmbouter and @dralley have very deep knowledge of the tasking system. I always ask them to review changes in those parts of the code.

I don't ask @asmacdo to review changes in the tasking system. I don't ask @bmbouter to review changes to viewsets/serializers.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But the commit bit isn't as much about the knowledge as about the trust: we trust the folks to do their best to approve or disapprove PRs.
Moreover asking other folks for a PR helps to spread the knowledge.
Also mind @asmacdo might shift focus one day, should the commit bit for a subdirectory follow his focus?
I'm sorry this doesn't seem a sufficient reason to me to accept the directory commit permissions.

Copy link
Contributor

@asmacdo asmacdo Jun 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the concept of the idea. I'd love to be able to review all the PRs that use DRF. But I do have some concerns.

  1. I agree with Milan's concern. I think the need for 2 reviewers seems like it would happen often, if only for an import statement change.
  2. How could we make it obvious who needed to review each chunk?
  3. PR review is one of the ways to learn an area we don't know as well, particularly when reviewing code written by one of the area specialists.
  4. For reviewers with more limited permissions, PR authors have a disincentive to ask for review, bu

t we want to encourage building up the review skills of newer committers.

If most of the review is done by the people who do review can review everything, we will avoid most of these concerns. The people with commit bit for the entire repo should be limited, but should still include everyone who has been working on it full time for a while. New or infrequent committers might be given access to particular areas.

* Vote end date. Must be 7 calendar days from now.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/now/from the application date?


To pass, a unanimous +1 vote is required from other committers in that area. Specifically:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/committers/commit bit holders/


* Adding a committer to a subsystem requires 100% vote participation from all existing maintainers
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/maintainers/current commit bit holders/ ; How about absences? I'd suggest reducing this to a majority w/o a -1 vote.

of that subsystem.
* Creating a first owner of a subsystem requires 100% vote participation from that repository's
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's a first owner?

committers.
* Adding a committer for a repository requires 100% vote participation from that repository's
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

subsystem != repository??

committers.

If a nomination does not pass, the candidate may be re-nominated after a cool down period of 60
days.


## Drawbacks

A single developer can block a new candidate. This could limit contribution unnecessarily and
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/developer/commit bit holder/

possibly create fairness issues.

Candidates might get waived through because casting a blocking vote can be uncomfortable.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/Candidates/Applicants/



## Alternatives

One alternative is to never add new committers. That is not a good option.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say this is redundant


Another alternative is to use hard metrics such as time involved, feature planned and implemented,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd appreciate different voting schemes in the alternatives esp. with the drawbacks you mentioned

bugs fixed, etc. Overall hard metric system can be gamed which is a major downside. Also a metrics
based process does not help unify the committer base like a unanimous vote would.


## Unresolved Questions

How will the commit bit be removed?
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is an important issue and deserves addressing in this document.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this specific pup is about 'commit bit assignment', i would expect to be written a separate pup which would address commit bit removal and relieve.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would be fine with either but even if it is a separate PUP, we should get it over and done with now I think.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assigning commit bit criteria change indeed needn't imply change in criteria removal.
Still I'd find it more safe against the: "oh I didn't realize the implications" kind of errors if both assigning and relieve criteria are specified in a single doc.
We're most likely talking about <= +20 slocs worth of change fwiw.