Re: TianoCore Community Meeting Minutes

[Laszlo Ersek <lersek@redhat.com>]

Hello Jeremiah,

On 10/19/18 18:09, Jeremiah Cox wrote:

> I'd like to better understand assertions of GitHub "lock in".  GitHub
> provides a comprehensive REST API that you can easily drive via Python
> ("pip install PyGithub").
> I recall one assertion that GitHub holds PR comments hostage, but it
> is trivial to dump out all Comments on all PRs in a repo.
>
>
>    g = Github( <access_token> )
>    repo = g.get_repo("chipsec/chipsec")
>    pulls = repo.get_pulls(sort='created', base='master')
>    for pr in pulls:
>       for comment in pr.get_review_comments():
>          print(pr.number)
>          print(comment.body)
>          ...
>
> Can folks help me better understand examples of GitHub "lock in"?  I
> think it would be trivial to author a daemon that listens for PRs and
> comments, and forward those to a mailing list, if that is preferred
> for archival.

> With respect to web-based, we believe it facilitates convenient,
> multi-platform access, and GitHub's REST APIs can be leveraged to
> provide a command-line driven experience.  I believe Sean's point was
> that GitHub provides a 1-stop shop for code reviews and issue tracking
> such that there is no need to spend TianoCore resources set up,
> maintain, and update separate Bugzilla and Gerrit services, VMs, OSs,
> & machines.  The REST APIs could be used to maintain a mailing list or
> other mirror.  I am not asserting that GitHub provides the best code
> review and issue tracker experience, our team finds Azure Dev Ops is
> superior, but GitHub is sufficient for most use cases while handling
> the infrastructure so that we can focus on getting things done.
>
> I think it would be helpful to construct a PROs/CONs table for each of
> the proposed end-to-end solution (source control, code review, gates,
> CI/CD, issue tracking, testing, ...).  Perhaps this table is on a wiki
> to enable faster iteration than emails on the DL.

> We think of "Pull Request" as a contribution & review process akin to
> GitHub, GitLab, or Azure DevOps.  Feedback, responses, automated
> check-in gates, and signoff can be handled within each PR ticket.
> Policies can be assigned, automated tests that must pass, enforcement
> of community wide rules (# of reviewers, reviewer list, all comments
> "resolved").  Tests could start as simple as successful build and over
> time could grow to a more comprehensive set of boot, functional,
> static analysis, ...
> By leveraging a popular hosting provider's built-in workflow we enjoy
> significant efficiencies.  Its fully documented and known to most. New
> capabilities are being added daily.  No TianoCore resources would be
> required to manage systems, OS patches, network issues, etc and can
> instead be focused on TianoCore code, building tests, and driving
> process improvements.

I don't have anything against GitHub specifically. I also don't have any
"favorite" WebUI (beyond Bugzilla, of course :) ). I'm concerned about a
number of general anti-patterns that might not be obvious, and might not
be easy to prevent with any given modern web application. Perhaps I can
word this as a personal wish list:

* Full email audit trail (all changes mailed out, even own ones), about
  patch submission, PRs, patch review comments, and bug tracker actions
  alike.

  One should be able to establish the full history of a bug or a pull
  request just from the emails. (This would be for human readers, not
  for machines.) Review comments made on the web, in reference to
  specific parts of a patch, should preserve that context in email form.

  The messages generated for a given bug or PR should form a thread on
  the mailing list. (Proper use of In-Reply-To and References headers.)

  The generated emails should be readable without HTML support (i.e. as
  plain text). As a counter-example for this, I'll mention JIRA --
  JIRA's emails are practically unreadable without HTML rendering
  support in the MUA.

  If a WebUI can offer such an email gateway as a feature, that's
  awesome. It is fine if the gateway is one-way only (web to email). The
  service should not be an external one however, in order to minimize
  email lossage when the WebUI works but the gateway goes offline for
  some reason. The gateway should be tightly integrated.

* The comment editing interface on the WebUI should support, in addition
  to the default rich editing interface, a plaintext mode that uses
  monospace font, interprets no markdown sequences, and preserves the
  exact layout of the plaintext comment (including indentation and line
  wrapping).

  Counter-examples abound. Perhaps somewhat unexpectedly, I'll call out
  Launchpad. Among other things:

  (a) Launchpad truncates comments it thinks are "too long", in the full
      bug display -- and then readers have to open the bug comment
      separately, to see it in its full length;

  (b) Launchpad inserts invisible breaks into words it thinks are "too
      long" -- and then, if someone double clicks a 40-nibble commit
      hash in the middle, they will not see it wholly selected; only a
      middle section will be selected between the hidden word breaks.
      The invisible word breaks will also allow commit hashes to be
      wrapped.

* A similar "stick to basics" criterion exists for commit message
  authoring, and display.

  Commit messages are to be authored with monospace font in mind, and 74
  character line length.

  Commit messages should not be reflowed or rewrapped automatically on
  display. BZ numbers and commit hash references may be highlighted and
  turned into actual links, but they should not be shortened or
  otherwise reformatted. The original formatting of a commit message is
  not a mindless process, and any WebUI should respect that, and
  preserve the original author's work, for display too.

* Old versions of patch series (pull requests) and their associated
  review comments must remain publicly available forever. No historical
  review comments must ever turn into dangling references on the WebUI,
  nor must old (rejected) pull requests (topic branches) ever be
  possible to delete (or to overwrite by force-pushing), even for the
  original author.

* No "squash on merge". Structuring a patch series is a first class
  development activity, and the git history is a first class aspect of
  the codebase.

* More generally, admins should have fine-grained control to disable
  unwanted WebUI features.

* "Edit by reviewer on merge" should be restricted to minimally invasive
  changes, and participants must agree on such actions up-front.

* I don't know how a WebUI can support "rebase by maintainer". On one
  hand, such a feature would have to be used very carefully (see the
  previous point); on the other hand, adding Reviewed-by and similar
  tags without it is not possible for the maintainer.

* Support for displaying and perhaps editing "git notes" (see
  git-notes(1)) would be a plus. They permit developers to attach
  patch-level changelogs (incremental change descriptions) to individual
  patches, such that these changelogs never make it to the final git
  history. Such notes are useful for repeated reviews, but not really
  useful for the long-term git history (which does not capture the
  discussion context / previous versions of the pull requests anyway).

Thanks!
Laszlo