Open Stack

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.


--
Cheers,
Christophe de Dinechin (IRC c3d)