On Thu, Apr 07, 2022 at 10:16:55AM +0100, Hunt, James O wrote:
Hi David,
Thanks for raising! Comments inline below, but I wonder if we should move this to a GitHub issue for wider discussion?
Well.. maybe. Like cleanup patches, I feel like GitHub is a poor match for wider policy discussions like this (unlike cleanup patches it's not something I feel ready to tackle). In particular, I'm not clear on how I'd address a github ticket to the right audience.
Le 2022-04-07T15:13:57+1000, David Gibson a écrit :
Hi everyone,
I believe this idea was raised briefly at a recent AC meeting, but here's a more concrete proposal.
RFC: A streamlined process for cleanup patches ==============================================
Rationale ---------
Currently, commiting a change to Kata generally requires both a Github issue and a linked Github pull request. This works for bugs found "in the wild" and for feature/enhancement requests: it makes sense to discuss the change that needs to happen before moving on to how to implement it.
However, this process works poorly for "cleanup" patches. That includes internal refactoring without changing external behaviour, and also bugs found by inspection where it's clear that the code doesn't behave as it should, but working backwards to find user inputs which would trigger the bug can be quite difficult. It's certainly possible to write an issue for such a fix, but it often ends up just re-iterating the same information as in the PR; the issue templates also don't help with it since they assume the report is coming from observation of behaviour, rather than inspection of code.
In addition cleanup patches don't work well with the procedural checks which require each PR to have a patch labelled as fixing a Github issue. A well written cleanup PR will often consist of a number of small patches which each incrementally improve the code quality, but none of which could be said specifically to "fix" a clearly delineated problem. This means that someone writing a cleanup PR has to either arbitrarily place the "fixes #" tag on the last patch (which is misleading), or place it on all of them which could lead to prematurely closing the issue if an incomplete series is merged my mistake.
To reduce barriers to people addressing technical debt in Kata, I therefore propose that we introduce a shorter process for reviewing and merging cleanup patches.
+1.
Guidelines ----------
We don't want to stop encouraging well written reports for externally observed bugs, or enhancement requests. So, we need some guidelines as to what constitutes a "cleanup" PR.
A PR which changes the code without changing any externally visible behaviour would certainly qualify. I think limiting to *only* that would be too restrictive though: that would disallow PRs which technically change behaviour but only in an obscure / unsupported / unimportant case. Likewise it would prevent fixing of bugs found by inspection - we want to encourage people to fix bugs, not slow them down with bureaucracy.
Here are some possible guidelines that admittedly leave some fuzzy room in the middle:
Considered a cleanup patch: * Internal refactoring with no external behaviour change * Rewording comments or documentation for clarity * Updates to developer facing documentation to match other cleanup changes
NOT considered a cleanup patch: * Added functionality * Anything requiring user facing documentation to be updated to match code changes
+1.
Process -------
There are several ways we could allow for a streamline cleanup process. Here are three options, which I'm soliciting opinions on:
Option 1 (no issue required)
* Cleanup PRs can be submitted with no accompanying Github issue * Such a PR must be tagged with a (new) 'cleanup' label
I like this option best, but a more flexible solution would be to require option 1 for cleanup work, and make option 3... optional, to provide further details if required ;)
That's a good point. Going for (1) doesn't preclude using (3) as well where it makes sense.
However, I'd rather we mark the commit (see below) than the PR since that way the Git history stores the details rather than only having it stored in GitHub metadata. We could get the benefit of both though if we wrote a new GitHub Action to automatically add a `cleanup` label when it finds a cleanup commit.
Ok, that makes sense. I think we should consider a PR a cleanup PR only if *every* commit in the PR is a cleanup patch. Including some preliminary cleanup patches before a patch adding functionality is a reasonable pattern, but shouldn't go through this process.
* Scripts would be adjusted to to require a "fixes" tag on a cleanup labelled PR
s/to to/to not/ presumably.
Thanks, corrected.
Advantages: - Minimum of overhead for small fixes Disadvantages: - Process would need to be documented elsewhere, with no ability for Github to guide people through it
True, but with good docs, I don't see that as an issue. But to help contributors who forget to add the crucial "fixes #" comment [1] (new contributors and seasoned developers in a hurry), we could tweak the static check GitHub action to display an error along the lines of:
"no fixes issue found - is this a cleanup PR? See <fixes-docs-url> and <cleanup-docs-url> for further details"
That's a good idea.
Option 2 (special issue template, minimal PR)
* A new issue template for Cleanup is added alongside enhancement request, bug fix etc. * PRs attached to a cleanup issue are allowed / expected to be minimal, simply linking the issue with no further description required (individual patches in the PR should still have good commit messages, of course)
Advantages: - Issue template can help guide people through the processz Disadvantages: - Still requires the extra step of creating both the issue and PR
Option 3 (long-lived cleanup issues)
* Cleanups can be attached to a specially labelled issue which describes not a specific problem, but rather a general area where cleanup / improvement might be desirable * PRs linked to a "cleanup" issue aren't expected to "fix" it, just to make some amount of improvement * Merging multiple PRs against the same cleanup issue would be normal * Scripts would be adjusted not to require a "fixes" tag on such a PR, but maybe a "improves" tag. Processing such a PR would *not* close the issue
Advantages: - Allows incremental improvements to be merged quickly, without requiring them to fully "fix" some delineated problem Disadvantages: - Unclear when the right time to (manually) close such an issue would be, because it's now "good enough"
Agreed that it's unclear when to close the issue. Some alternatives:
- Reference a wiki page URL. - Reference an issue in a new "cleanup" repo that is simply for tracking.
That seems like it unnecessarily adds moving parts, which is kind of the opposite of what I'm aiming for.
- Reference a GitHub project URL.
These would all remove the possibility of the issue being closed inadvertently by a cleanup PR but still don't solve the problem of when to consider the work "done". This does make me wonder if we need a new GitHub team to oversee such cleanup work and decide when the chosen tracking device needs to be closed.
Right. For the time being I feel like we should go for Option 1, and consider adding the option for "gradual tracking" issues later.
## Conventional commits
We've discussed the topic of conventional commits [2] previously [3], but now might be a good time to revive that conversation. Our patch format [1] isn't that dissimilar to the conventional commits format already. Plus, we should be able to remove checkcommits [4] from our CI process and rely entirely on the GHA commit checker [5]. Then all we'd need to do is get [5] to support a variant [6] of conventional commits **optionally**.
For cleanup PRs, rather than something like this:
``` build: Fix typo in Makefile
Correct a spelling mistake in the top-level Makefile.
Fixes: #1234.
Signed-off-by: $name <$email> ```
... we could (make [5]) allow:
``` cleanup: build: Fix typo in Makefile
Correct a spelling mistake in the top-level Makefile.
Signed-off-by: $name <$email> ```
... or optionally:
``` cleanup: build: Fix typo in Makefile
Correct a spelling mistake in the top-level Makefile.
Improves: $url
Signed-off-by: $name <$email> ```
In other words, just add a "cleanup:" conventional commits "type" before our subsystem prefix [1].
Seems reasonable. I don't really want to stall this with deciding whether we want to take all of the conventional commits stuff. But we might as well pick a way of tagging cleanup commits that's compatible or close in case we pick up more of those conventions in future. -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson