[GHC DevOps Group] CircleCI job accounting question

[Manuel M T Chakravarty]

> Am 13.12.2017 um 02:54 schrieb Ben Gamari <ben at well-typed.com>:
> 
> Manuel M T Chakravarty <manuel.chakravarty at tweag.io> writes:
>>> Am 12.12.2017 um 02:22 schrieb Ben Gamari <ben at well-typed.com>:
>>> 
>>> I personally think that we should strive for your first point (every
>>> commit should be validate-clean) before attempting to tackle your
>>> second. I, for one, am rather skeptical that putting all of your patches
>>> through review would significantly affect quality.
>> 
>> I completely agree.
>> 
> On re-reading what I wrote above, I realize that it was a bit unclear.
> To clarify, the sentence
> 
>>> I, for one, am rather skeptical that putting all of your patches
>>> through review would significantly affect quality.
> 
> was intended to mean "I am not convinced that quality would improve as a
> result of review". Is this what you are agreeing with?

Yes.

>> So, what is preventing us from disabling direct pushes to master and
>> requiring all contributions to go through a PR or Differential?
>> 
>> PRs and Differentials are squashed on merging to master and the whole
>> problem (with CircleCI building only heads of commit groups) just
>> disappears. I believe that this is the usual approach.
>> 
> We generally don't want to squash. Those who typically commit directly
> generally master generally take pains to ensure that their commit
> history is sensible. This history is valuable, both for the future
> readers of the code as well as later bisection; projecting it out is
> quite undesirable.
> 
> We could indeed require that Simon, et al. open Differentials for each
> of their commits. However, as I mentioned earlier it doesn't seem likely
> that putting these commits through rigorous review would be fruitful
> relative to the work it would require; these differential would be just
> be for merging. Unfortunately, the cost of creating a differential is
> non-trivial and I'm not certain that we want to make Simon pay it for
> every commit.
> 
> Admittedly this is where a Git-based workflows have a bit of an
> advantage: it is much more convenient to merge a group of commits in a
> structure-preserving manner via a git branch than a stack of
> differentials.

Sorry for being unclear, but I was not suggesting to put Simon’s commits in Differentials to review them, but to make everything go through the same funnel, triggering CI on all the commits it ought to be triggered on.

Generally, I think, direct commits to master are bad process (and this why this is not well supported by CircleCI either). Two key points of CI are

* vet all incoming changes before they land in master and
* automate all validation and artefact generation. 

But pushing directly to master, we compromise both. There is no vetting. Instead, we rely on the developer to do the right thing and validate locally. This means (a) we open ourselves to mistakes (which as SPJ wrote, do happen sometimes) and (b) we replace automation by a manual process.

The right thing to do is to handle all commits in the same way. They all go through CI and nobody needs to validate locally. As a bonus we solve the problem that every commit gets validated, too (because all go through CI anyway).

The hold up seems to be that Phabricator creates an overhead, which has prompted the use of the loophole (= direct push to master).

How about the following solution? Everything that is directly pushed to master currently, is being pushed to GitHub and goes through a PR.

After all, the main criticism of GitHub PRs seems to be about code reviews being nicer on Phabricator and we don’t want code review on the direct pushes to master anyway. So, why not use GitHub for this?

Cheers,
Manuel

>> If the outstanding issue is that you combine multiple contributions
>> from contributors and manually valid them to ensure they are not just
>> individually sound, but also in combination, we might want to consider
>> 
>>  https://bors.tech <https://bors.tech/>
>> 
>> which is exactly for that kind of thing (and apparently used by Rust).
> 
> Indeed, I'm familiar with bors. Unfortunately it's quite GitHub-centric,
> so at for the moment it's not a viable option.
> 
> Cheers,
> 
> - Ben
> _______________________________________________
> Ghc-devops-group mailing list
> Ghc-devops-group at haskell.org
> https://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devops-group