Open Stack

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.

That too..

> Not sure how to reconcile the two points of view ;-) I will let
> David decide what to do.

At present my intention is to fold in what feedback I do get here, the
put a refined proposal onto github.  Any advice on how to address that
future issue to the right people is appreciated.

> > 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?

Uh.. depends how major, I guess?

> 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).

That's admittedly a bit fuzzy.  I think requiring changes to
functional or integration tests would generally disqualify a PR from
being a cleanup.  I can certainly imagine exceptions to that, but I
think they're rare enough that we wouldn't lose a lot of benefit if we
enfoced it.

To be clear, the red flag is requiring *synchronized* code and
functional/integration test changes.  Both cleaning up code in a way
that it works against existing tests and cleaning up tests so they
work against existing code are potentially fine cleanups.

In contrast requiring synchronized code and *unit* test changes would
usually be fine for cleanups.  Especially since IMO, Kata currently
has rather a lot of poorly designed unit tests which test details of
the implementation rather than the outcome of the code in question.

That said, changes which asignificantly lter widely used internal
interfaces might be borderline for "cleanup" categorization.  I'm
thinking size of conceptual change here - I'd consider changing the
signature of a function with matching updates to 1000 callsites fine,
providing those 1000 callsite changes are all trivial (e.g. dropping a
parameter that can be derived from others or where all the callers
passed the same thing).

> >> 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'd consider that a cleanup candidate if found by inspection.
Obviously such a thing could also be reported as a bug without patch,
which would follow the existing process.

> 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)

Agreed.

> 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'm not familiar with those changes but "in support of Darwin"
suggests added functionality, which would disqualify it as a cleanup.

> 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.

Right.  I don't think we can eliminate judegement calls from this

> Also, what about one-line fixes for obvious typos / minor errors?
> Not strictly a cleanup.

I'd consider that a candidate, whether or not it's "strictly" anything.

> >> 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/20220413/1dae22d0/attachment-0001.sig>