What does done mean?

[Claudenw]

This is to open a discussion about when code is 'done' enough that it can be merged into the main branch. As this is a discussion, please add comments. The solution here is only my opinion and like belly buttons everyone has one so please present it here.

what is required of the pull request

I think that there are several conditions for code to be accepted:

1. Whatever is being added or changed has test cases.
2. Whatever is being added or changed has documentation in the code that can be extracted with cargo doc or whatever the standard documentation generator is for the language of the code.
3. code files must have license information (and copyright?) at the top of the file. We can find tools that will check this for us.
4. Code must be formatted with cargo fmt or whatever is the standard code formatter for the language of the code.
5. changes are in a pull request.

how the process works

Once the above have been satisfied then the pull request needs to be reviewed by developers experienced in the project. This brings up the issue of a meritocracy. I recommend a quick read of the Apache Foundation project roles documentation (1 page). Then come back here.

Using the Apache terminology.

The current active developers and appropriate managers make up the PMC as of now. Anyone else in the company that can see the repository are users. Someone like myself, with minimal experience on the project, would be considered a contributor/developer. At some point in the future, when I get enough experience, the PMC can vote to promote me to a committer. Note that only committers and PMC members can merge code. To become a committer one has to show they know what is expected (list above) and they routinely submit code that meets the requirements; and they have to nominated by a PMC member and the PMC must vote to accept them.

I think the process for accepting the code is:

1. A developer makes a pull request. This true of all cases from PMC on down.
2. A committer reviews the pull request. If it is simple (e.g. typo, simple bug, documentation change) they can merge it and close the pull request and the process ends here.
3. If it is not a simple pull request, the committer, the developer(s) and one other committer review the changes. (keep in mind that all PMC members are committers). This review can be real time via phone or video or chat, or it can be asynchronous via comments in the pull request. HOWEVER, any comments made during any real time interactions must be documented in the request. If they are not documented, they didn't happen. If the code passes the review it is merged and the process ends.
4. If the code does not pass, the developer may make recommended changes and request another review. So the process works through requested changes in the same pull request.

[benbromhead]

I think this is a great basis to start from in terms of our definition of done.

I would add a few requirements from my sides / we need to do from our own internal Instaclustr reqs:

• 2 reviewers for a PR. The level of effort of each review can be asymetrical.
• main must be "releaseable" at any point in time... this means we might need to have a think about how we break apart large feature work that need multiple PRs to review and understand. Do they get commited and reviewed to a feature branch? I'm not sure what the best way to do this is. A slightly less strict version of this, would be "main" must always "pass tests"... but "releasable" would be my preference.
• cargo fmt is good, I think a clean cargo clippy run is also a good idea (I'm also pretty comfortable with turning off various clippy warnings if needed / documented).

cc @XA21X @rukai @conorbros @johndelcastillo @slater-ben
Would be great to get some input

[conorbros]

Adding a pre-commit hook for things that we want checked before every commit (e.g. cargo fmt and cargo clippy) would be good.

It will require going through the codebase at the moment and fixing all the clippy warnings before we can start using it.

[rukai]

Code files must have license information (and copyright?) at the top of the file. We can find tools that will check this for us.

Is this really needed?
Its an extra maintenance burden and I cant recall a single rust project that does this.
AFAIK having a license file in the root of the repo and mentioning the license in the readme should be enough.
Maybe a question for Justin?

+1 for main always being releasable.
Although I think we need to further define "releasable", I think it would be fine to just ensure we arent causing any regressions in existing functionality but new functionality that hasnt been included in a release yet can be checked in half-baked with the expectation that a release wont occur until its in a releasable state or the functionality is disabled.

Enforcing warnings/clippy is a slightly contentious issue.

• https://www.reddit.com/r/rust/comments/f5xpib/psa_denywarnings_is_actively_harmful/
• https://github.com/rust-unofficial/patterns/blob/main/anti_patterns/deny-warnings.md

A possible solution is enforcing rustc warnings and specific clippy lints purely in CI.
rustc and clippy make no guarantees to the stability of warnings, existing warnings/lints can be changed to detect cases that they did not previously detect and introduce new warnings/lints at any time.
So this still has the downside that CI will break from time to time.
And rustc lacks a way to fail CI on only specific warnings.
But maybe we are willing to accept that to ensure our codebase remains clean.

We could avoid the CI breakage entirely by pinning to a specific rust stable release in our rust-toolchain.toml but that means we need to now manually update our rust version when new releases occur.
So I would only pin our rust release if we find the CI breakage too frequent.

If we start using clippy, the way to do it would be to start with no lints and then make seperate PRs to enable + cleanup lints that we want to start using one by one (or at least in small batches)

It seems git hooks are configured locally so we cant ensure a uniform setup for everyone with them, but individual users are free to setup as they please.

[benbromhead]

It largely depends on the license and the requirements of the software foundation you donate a project to. ASF for example requires it. GPLv3 strongly recommends something at the top etc. It's generally considered a "good idea", but I've never seen a counterfactual from someone not doing it and having a bad time.

It's also a nice thing to do so folks can include single source files, or for license compliance / discovery tools.

For the moment I'm not too worried about addressing this right now (as long as our copyright, license and notice files are in order and communicate clearly in the repo root), as its more work and we don't need to do it right away. Let's address when we start thinking about a foundation donation.

Code files must have license information (and copyright?) at the top of the file. We can find tools that will check this for us.

Is this really needed?
Its an extra maintenance burden and I cant recall a single rust project that does this.
AFAIK having a license file in the root of the repo and mentioning the license in the readme should be enough.
Maybe a question for Justin?

[justinmclean]

Re file headers I can provide multiple examples of where people have not done it and it turned out to be a bad idea. The main issue is there is no way to tell if that file is a 3rd party file (which could be under another license) or a file created by the project. Some business will not use code that doesn't have clear IP provenance or licenses. Some business use automated tools to check this. From a quick look I can see some 3rd party code in the code base that is not clearly identified and I'm also unsure of it license. IDEs (and other tools) can automatically add headers.

Most of the discussion above has focused on a RTC (review then commit) system, a large number of Apache projects use CTR (commit then review). That may be worth considering, projects that do this more effort it put in at release time to make sure everything works as expected and there no IP issues. On the plus side it makes it easier for people to contribute, and reduces the need to find and beg people to review your contributions.

Re the PMC structure, how will the PMC communicate and vote on new committers? There's good reasons for not doing that in the open. At the ASF this usually done in a private email list, and new PMC members get access to this list when they are made a PMC member. Perhaps a private slack channel rather than a mailing list?

If we are considering a PMC/committer governance model we may also want to think about how that works for releases. e.g At Apache the PMC votes on a release over 72 hours and you need 3 +1 votes and more +1s than -1s to make a release. A -1 vote on it's own is not a veto.

The other thing to consider is ICLAs. I think it would be best to require all contributors to sign an Apache style ICLA before any of their contributions can be accepted. Not doing this will cause a huge amount of work when the code base does land at a foundation or if we want to change the license at a later date. (Basically you would need to try and contact all previous contributor to get permission.) Business are also more likely to use our code as it gives some assurances around IP. Rather than manually doing this I'd suggest using something like https://github.com/cla-assistant/cla-assistant. Note that Apache style ICLAs don't ask for copyright assignment, but just permission to licence and distribute the code contributed.

[benbromhead]

As discussed with folks previously (but I don't think written in this thread) the governance model is largely going to be a "temporary benevelant dictator" which will be myself.

Changes, releases, assignment of "write" permissions to the repo for new users will be my decision.
Merges by existing people with "write" permissions to the repo simply require 2 reviews before commiting at the moment. I don't need to be explicitly one of those people.

There is also a ticket for automating ICLAs -> #271 it is a priority but not on the critical path for open sourcing.

[justinmclean]

Not an issue now, but I'll just point out that the ASF doesn't allow projects to have dictators, benevolent or otherwise.