Open Stack

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.katacontainers.io/pipermail/kata-dev/attachments/20220413/7533bee8/attachment-0001.sig>