Open Stack

As suggested, I've now proposed this as a github issue

https://github.com/kata-containers/kata-containers/issues/4099

On Fri, Apr 08, 2022 at 02:07:45PM +0200, Christophe de Dinechin wrote:
> 
> On 2022-04-07 at 10:16 +01, "Hunt, James O" <james.o.hunt at intel.com> wrote...
> > Hi David,
> >
> > Thanks for raising! Comments inline below, but I wonder if we should
> > move this to a GitHub issue for wider discussion?
> 
> I made the same comment to David. However, without speaking for David, some
> developers coming from a kernel / qemu / "old" background are more at ease
> with mail-based discussion, notably because it makes it easier to quote
> extensively. Not sure how to reconcile the two points of view ;-) I will let
> David decide what to do.
> 
> >
> > 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.
> 
> I agree. In general, if the PR only repeats the information found in the
> issue, or if you fill an issue just to be able to submit a PR, this
> indicates that we are just creating process overhead.
> 
> 
> >> 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.
> 
> Indeed.
> 
> 
> >> 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.
> 
> +1 too
> 
> >
> >> 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.
> 
> Would that include a major refactoring of the code?
> 
> I would be a bit more restrictive here, add some kind of size or simplicity
> criterion. Maybe what I do not understand is what you mean with "externally
> visible". Do you mean outside of the code being affected (i.e. unit test
> level), or external to the program (i.e. functional test).
> 
> 
> >> 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.
> 
> What about removing a case where there is an agent panic and replace it with
> a proper error message? We don't document all detailed errors.
> 
> I would consider the following as OK:
> - Improvements in error handling or error messages?
> - Change in some local algorithm for a more efficient one (e.g. a linear
>   search with a binary search)
> 
> What I am not sure about is more significant refactoring, like the rather
> big changes done recently in support of Darwin. I would argue that such
> large changes need issues ahead of time to explain what is being done.
> 
> I can't come up with a really precise definition, though. Common sense
> applies. Maybe a simple rule would be that someone reviewing could state
> that a PR is too complicated and that an issue needs to be opened for it.
> 
> Also, what about one-line fixes for obvious typos / minor errors?
> Not strictly a cleanup.
> 
> 
> >
> >>
> >> 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 ;)
> >
> > 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.
> 
> Funny that I recall making almost the exact same comment to David when we
> discussed this ;-)
> 
> The cleanup label is useful because that's how you search things on GitHub.
> But I believe that "cleanup:" (for pure cleanup) or "minor:" (for one-line
> fixes or similar) in the first line of the git commit.
> 
> >
> >>     * Scripts would be adjusted to to require a "fixes" tag on a
> >>       cleanup labelled PR
> >
> > s/to to/to not/ presumably.
> >
> >>
> >>     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"
> 
> +1
> 
> 
> >
> >>   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
> 
> And not a win at all for single-patch fixes (even worse for single-line fixes)
> 
> >>
> >>   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
> 
> I like the idea of the "improves" tag, irrespective of your proposal.
> Or "Issue: #1234"... (shorter)
> 
> For example, if there are multiple commits in a PR, those that make
> progress, but don't fix, could be "Issue: #1234"
> 
> 
> >>
> >>     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"
> 
> Also makes it a bit harder to know when something is closed.
> 
> - With a PR, we know that the PR closes something, even with multiple commits
> - With an ongoing issue, this is harder to see in the history.
> 
> >
> > 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.
> > - 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.
> >
> > ## 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].
> 
> I agree, but I would distinguish:
> 
> - cleanup: Change without functional impact (variable rename, comments, doc,
>   spelling, ...)
> - minor: A minor fix
> - refactor: A functional change to improve the code
> 
> 
> >
> > Cheers,
> >
> > James
> >
> > [1] - https://github.com/kata-containers/community/blob/main/CONTRIBUTING.md#patch-format
> > [2] - https://www.conventionalcommits.org
> > [3] - https://github.com/kata-containers/kata-containers/issues/195
> > [4] - https://github.com/kata-containers/tests/issues/4596
> > [5] -
> > https://github.com/kata-containers/kata-containers/blob/main/.github/workflows/commit-message-check.yaml
> > [6] - The Conventional commits term for our "subsystem" is "scope" which is put
> > in brackets rather than being a colon prefix as Kata currently uses.
> 
> 

-- 
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/20220414/910e1975/attachment.sig>