Obstacles for using GitLab CI

classic Classic list List threaded Threaded
16 messages Options
Reply | Threaded
Open this post in threaded view
|

Obstacles for using GitLab CI

Jonas Hahnfeld
Hi all,

as discussed before the migration, we might want to look into using a
CI system. Foremost this would help James who is currently still
testing patches manually. At least the doc build can and should be
completely automatic.
Additionally GitLab has a feature called "Merge Trains", see [1] for
the documentation. In short this is a queue of merge requests ready to
be merged (Patch::push in our speak). For each of them, GitLab verifies
that the potentially merged result passes testing. Only afterwards the
change is allowed to hit the target branch. This is basically what the
current patchy-staging setup ensures, only at the granularity of merge
requests.

That was the nice part in theory. The bad news is that this feature (at
the moment) doesn't work well with "Fast-forward merges". In fact,
GitLab requires you to rebase your branch (there's a button in the web
interface) before merging is allowed.
As you can easily imagine, this renders merge trains unusable: Say you
have two merge requests and rebased the first to add it to the train.
Now you still have to wait for the merge to complete before you can
rebase the second. GitLab devs intend to make this work better in the
future [2][3], but that's already in discussion for some time.

So the only way out (right now) would be to merge patches instead of
ensuring a linear history. This would be radically different from the
current process, at the advantage of being "more standard". What do you
think about this? (To be honest, I'm not even sure if I like it. But I
do like the prospect of having automated testing.)

Regards
Jonas


1: https://docs.gitlab.com/ee/ci/merge_request_pipelines/pipelines_for_merged_results/merge_trains/
2: https://gitlab.com/gitlab-org/gitlab/-/issues/35628
3: https://gitlab.com/gitlab-org/gitlab/-/issues/895

signature.asc (499 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Obstacles for using GitLab CI

David Kastrup
Jonas Hahnfeld <[hidden email]> writes:

> Hi all,
>
> as discussed before the migration, we might want to look into using a
> CI system. Foremost this would help James who is currently still
> testing patches manually. At least the doc build can and should be
> completely automatic.
> Additionally GitLab has a feature called "Merge Trains", see [1] for
> the documentation. In short this is a queue of merge requests ready to
> be merged (Patch::push in our speak). For each of them, GitLab verifies
> that the potentially merged result passes testing. Only afterwards the
> change is allowed to hit the target branch. This is basically what the
> current patchy-staging setup ensures, only at the granularity of merge
> requests.
>
> That was the nice part in theory. The bad news is that this feature (at
> the moment) doesn't work well with "Fast-forward merges". In fact,
> GitLab requires you to rebase your branch (there's a button in the web
> interface) before merging is allowed.
> As you can easily imagine, this renders merge trains unusable: Say you
> have two merge requests and rebased the first to add it to the train.
> Now you still have to wait for the merge to complete before you can
> rebase the second.

Uh, isn't that exactly what we have to do with regard to staging?  The
only difference is when you take a dare and rebase on a staging branch
version that has not yet made it to master.  Depending on how testing
turns out, your rebase may get thrown out along with the actual culprit
of test failure.

> So the only way out (right now) would be to merge patches instead of
> ensuring a linear history. This would be radically different from the
> current process, at the advantage of being "more standard". What do
> you think about this? (To be honest, I'm not even sure if I like
> it. But I do like the prospect of having automated testing.)

At the current point of time, our pipeline does not tend to be all that
full I think.  We are not at Linux kernel levels of participation...

--
David Kastrup

Reply | Threaded
Open this post in threaded view
|

Re: Obstacles for using GitLab CI

Jonas Hahnfeld
Am Mittwoch, den 13.05.2020, 21:54 +0200 schrieb David Kastrup:

> Jonas Hahnfeld <[hidden email]> writes:
> > Hi all,
> >
> > as discussed before the migration, we might want to look into using a
> > CI system. Foremost this would help James who is currently still
> > testing patches manually. At least the doc build can and should be
> > completely automatic.
> > Additionally GitLab has a feature called "Merge Trains", see [1] for
> > the documentation. In short this is a queue of merge requests ready to
> > be merged (Patch::push in our speak). For each of them, GitLab verifies
> > that the potentially merged result passes testing. Only afterwards the
> > change is allowed to hit the target branch. This is basically what the
> > current patchy-staging setup ensures, only at the granularity of merge
> > requests.
> >
> > That was the nice part in theory. The bad news is that this feature (at
> > the moment) doesn't work well with "Fast-forward merges". In fact,
> > GitLab requires you to rebase your branch (there's a button in the web
> > interface) before merging is allowed.
> > As you can easily imagine, this renders merge trains unusable: Say you
> > have two merge requests and rebased the first to add it to the train.
> > Now you still have to wait for the merge to complete before you can
> > rebase the second.
>
> Uh, isn't that exactly what we have to do with regard to staging?  The
> only difference is when you take a dare and rebase on a staging branch
> version that has not yet made it to master.  Depending on how testing
> turns out, your rebase may get thrown out along with the actual culprit
> of test failure.
Maybe you're right and I was just overly enthusiastic about getting a
nice queuing mechanism...
In case we only want to apply one merge request at a time, the whole
process gets a lot easier: We don't need a train, but just tell GitLab
that "Pipelines must succeed" before merging is allowed. Together with
only fast-forward merges, this ensures testing of the actual commit
before it hits master. Sounds good from a policy perspective?

> > So the only way out (right now) would be to merge patches instead of
> > ensuring a linear history. This would be radically different from the
> > current process, at the advantage of being "more standard". What do
> > you think about this? (To be honest, I'm not even sure if I like
> > it. But I do like the prospect of having automated testing.)
>
> At the current point of time, our pipeline does not tend to be all that
> full I think.  We are not at Linux kernel levels of participation...

No, you're probably right. It's only a bit more bothersome if you have
multiple changes to submit from the same countdown.

Jonas

signature.asc (499 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Obstacles for using GitLab CI

David Kastrup
Jonas Hahnfeld <[hidden email]> writes:

> Am Mittwoch, den 13.05.2020, 21:54 +0200 schrieb David Kastrup:
>> Jonas Hahnfeld <[hidden email]> writes:
>> > Hi all,
>> >
>> > as discussed before the migration, we might want to look into using a
>> > CI system. Foremost this would help James who is currently still
>> > testing patches manually. At least the doc build can and should be
>> > completely automatic.
>> > Additionally GitLab has a feature called "Merge Trains", see [1] for
>> > the documentation. In short this is a queue of merge requests ready to
>> > be merged (Patch::push in our speak). For each of them, GitLab verifies
>> > that the potentially merged result passes testing. Only afterwards the
>> > change is allowed to hit the target branch. This is basically what the
>> > current patchy-staging setup ensures, only at the granularity of merge
>> > requests.
>> >
>> > That was the nice part in theory. The bad news is that this feature (at
>> > the moment) doesn't work well with "Fast-forward merges". In fact,
>> > GitLab requires you to rebase your branch (there's a button in the web
>> > interface) before merging is allowed.
>> > As you can easily imagine, this renders merge trains unusable: Say you
>> > have two merge requests and rebased the first to add it to the train.
>> > Now you still have to wait for the merge to complete before you can
>> > rebase the second.
>>
>> Uh, isn't that exactly what we have to do with regard to staging?  The
>> only difference is when you take a dare and rebase on a staging branch
>> version that has not yet made it to master.  Depending on how testing
>> turns out, your rebase may get thrown out along with the actual culprit
>> of test failure.
>
> Maybe you're right and I was just overly enthusiastic about getting a
> nice queuing mechanism...
> In case we only want to apply one merge request at a time, the whole
> process gets a lot easier: We don't need a train, but just tell GitLab
> that "Pipelines must succeed" before merging is allowed. Together with
> only fast-forward merges, this ensures testing of the actual commit
> before it hits master. Sounds good from a policy perspective?
>
>> > So the only way out (right now) would be to merge patches instead of
>> > ensuring a linear history. This would be radically different from the
>> > current process, at the advantage of being "more standard". What do
>> > you think about this? (To be honest, I'm not even sure if I like
>> > it. But I do like the prospect of having automated testing.)
>>
>> At the current point of time, our pipeline does not tend to be all that
>> full I think.  We are not at Linux kernel levels of participation...
>
> No, you're probably right. It's only a bit more bothersome if you have
> multiple changes to submit from the same countdown.

The real problem starts when people don't manage to get their patch in
because the pipeline is always busy.  But if we are there, the free CI
plan will most certainly not do the trick anymore.

--
David Kastrup

Reply | Threaded
Open this post in threaded view
|

Re: Obstacles for using GitLab CI

Dan Eble
On May 13, 2020, at 17:13, David Kastrup <[hidden email]> wrote:
>>> At the current point of time, our pipeline does not tend to be all that
>>> full I think.  We are not at Linux kernel levels of participation...
>>
>> No, you're probably right. It's only a bit more bothersome if you have
>> multiple changes to submit from the same countdown.
>
> The real problem starts when people don't manage to get their patch in
> because the pipeline is always busy.  But if we are there, the free CI
> plan will most certainly not do the trick anymore.

One thing is clear: we need to make sure we don't abuse James' generosity, so I'm willing to try whatever system you think will be the fairest all around, even if it might be a bit "bothersome" as an individual submitter from time to time.

Dan


Reply | Threaded
Open this post in threaded view
|

Re: Obstacles for using GitLab CI

Urs Liska-3
Am Mittwoch, den 13.05.2020, 22:33 -0400 schrieb Dan Eble:

> On May 13, 2020, at 17:13, David Kastrup <[hidden email]> wrote:
> > > > At the current point of time, our pipeline does not tend to be
> > > > all that
> > > > full I think.  We are not at Linux kernel levels of
> > > > participation...
> > >
> > > No, you're probably right. It's only a bit more bothersome if you
> > > have
> > > multiple changes to submit from the same countdown.
> >
> > The real problem starts when people don't manage to get their patch
> > in
> > because the pipeline is always busy.  But if we are there, the free
> > CI
> > plan will most certainly not do the trick anymore.
>
> One thing is clear: we need to make sure we don't abuse James'
> generosity, so I'm willing to try whatever system you think will be
> the fairest all around, even if it might be a bit "bothersome" as an
> individual submitter from time to time.
>

That's true.
But I'd like to disencourage (can you say that?) using our small
contribution frequency as an argument here. Making it easier to
contribute and as a consequence have more contribution was a major
motivation behind all this. So while we'll surely never get to a Linux
kernel situation having a solution that scales to some activity
substantially increased from what we have now should be a requirement.

Urs

> —
> Dan
>
>


Reply | Threaded
Open this post in threaded view
|

Re: Obstacles for using GitLab CI

Jonas Hahnfeld
Am Donnerstag, den 14.05.2020, 08:20 +0200 schrieb Urs Liska:

> Am Mittwoch, den 13.05.2020, 22:33 -0400 schrieb Dan Eble:
> > On May 13, 2020, at 17:13, David Kastrup <[hidden email]> wrote:
> > > > > At the current point of time, our pipeline does not tend to be
> > > > > all that full I think.  We are not at Linux kernel levels of
> > > > > participation...
> > > >
> > > > No, you're probably right. It's only a bit more bothersome if you
> > > > have multiple changes to submit from the same countdown.
> > >
> > > The real problem starts when people don't manage to get their patch
> > > in because the pipeline is always busy.  But if we are there, the free
> > > CI plan will most certainly not do the trick anymore.
> >
> > One thing is clear: we need to make sure we don't abuse James'
> > generosity, so I'm willing to try whatever system you think will be
> > the fairest all around, even if it might be a bit "bothersome" as an
> > individual submitter from time to time.
>
> That's true.
> But I'd like to disencourage (can you say that?) using our small
> contribution frequency as an argument here. Making it easier to
> contribute and as a consequence have more contribution was a major
> motivation behind all this. So while we'll surely never get to a Linux
> kernel situation having a solution that scales to some activity
> substantially increased from what we have now should be a requirement.
Should this really become a problem and GitLab not offer a solution by
then, we can of course still build our own queuing solution: Invent
something like Patch::merge and let a cron job periodically check for
merge requests with this label and apply them. This should be doable
via the API (did I say I love documented API endpoints?).

Thanks for all the feedback so far, I'll then work to propose something
simple that can at least get us started. Afterwards we can work from
there.

Jonas

signature.asc (499 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Obstacles for using GitLab CI

James Lowe-7

On 14/05/2020 07:58, Jonas Hahnfeld wrote:
> Thanks for all the feedback so far, I'll then work to propose something
> simple that can at least get us started. Afterwards we can work from
> there.

So for now I can just manually carry on running patchy-merge on staging
as needed?

James


Reply | Threaded
Open this post in threaded view
|

Re: Obstacles for using GitLab CI

James Lowe-7
In reply to this post by Jonas Hahnfeld
On a more general question, and not really understanding how this CI
workflow will change 'contexturally' what we do, so apologies for if
what I am about to say is ignorant, but are we still taking the
'master-must-always-be-good' /
'staging-can-be-bad-because-we-can-force-delete-not-just-revert' approach?

Rather than just automate everything and if something brakes we checkin
a reversion which then makes the tree not 100% compilable?

I've liked that approach we take so far and it always instills a level
of confidence about master.

James


Reply | Threaded
Open this post in threaded view
|

Re: Obstacles for using GitLab CI

Jonas Hahnfeld
In reply to this post by James Lowe-7
Am Donnerstag, den 14.05.2020, 08:05 +0100 schrieb James:
> On 14/05/2020 07:58, Jonas Hahnfeld wrote:
> > Thanks for all the feedback so far, I'll then work to propose something
> > simple that can at least get us started. Afterwards we can work from
> > there.
>
> So for now I can just manually carry on running patchy-merge on staging
> as needed?

Yes, for now everything stays the way it is, with the known patchy runs
to get commits from staging to master.

Jonas

signature.asc (499 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Obstacles for using GitLab CI

Jonas Hahnfeld
In reply to this post by James Lowe-7
Am Donnerstag, den 14.05.2020, 08:09 +0100 schrieb James:

> On a more general question, and not really understanding how this CI
> workflow will change 'contexturally' what we do, so apologies for if
> what I am about to say is ignorant, but are we still taking the
> 'master-must-always-be-good' /
> 'staging-can-be-bad-because-we-can-force-delete-not-just-revert' approach?
>
> Rather than just automate everything and if something brakes we checkin
> a reversion which then makes the tree not 100% compilable?
>
> I've liked that approach we take so far and it always instills a level
> of confidence about master.
We would keep 'master-must-always-be-good' but without having an
explicit staging branch that we need to revert. Instead we ask GitLab
to only add commits to master after they have passed testing. So
staging + patchy is basically replaced by merge request + GitLab
verifying the result of CI. My proposal will hopefully clear this up
with concrete examples.

Jonas

signature.asc (499 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Obstacles for using GitLab CI

Carl Sorensen-3


On 5/14/20, 2:31 AM, "lilypond-devel on behalf of Jonas Hahnfeld" <lilypond-devel-bounces+c_sorensen=[hidden email] on behalf of [hidden email]> wrote:

Am Donnerstag, den 14.05.2020, 08:09 +0100 schrieb James:

> On a more general question, and not really understanding how this CI
> workflow will change 'contexturally' what we do, so apologies for if
> what I am about to say is ignorant, but are we still taking the
> 'master-must-always-be-good' /
> 'staging-can-be-bad-because-we-can-force-delete-not-just-revert' approach?
>
> Rather than just automate everything and if something brakes we checkin
> a reversion which then makes the tree not 100% compilable?
>
> I've liked that approach we take so far and it always instills a level
> of confidence about master.

We would keep 'master-must-always-be-good' but without having an
explicit staging branch that we need to revert. Instead we ask GitLab
to only add commits to master after they have passed testing. So
staging + patchy is basically replaced by merge request + GitLab
verifying the result of CI. My proposal will hopefully clear this up
with concrete examples.

-> CS  I hope we can get back to the previous mode we worked in that not only is master always good, but that every commit on master is known to work.

-> CS  As I have been verifying issues fixed for 2.20.1 I have found a number of issues that were resolved with 2, 3, 4, or as many as 7 commits.  As far as I know, we have no testing that indicates each of these commits builds correctly, only that the full set of commits builds correctly.  We made a conscious decision to require only single-commit fixes so that every commit on master would build, and we could use git-bisect effectively to identify regressions.

-> CS  I know that we had some discussion about how it is much more convenient to have multiple small commits for reviewing, and I can see that in nearly every case the multiple commits were aimed at helping reviewers understand the patches, which I think is a great thing.  I'd be happy to keep multiple commits for reviewing, but I believe we need to ensure that every commit on master builds properly to support git-bisect.

-> CS  I hope that we can make our CI/merge process function such that either we run the CI on every commit in a merge request or we rebase/squash/make a single merge commit so that the intermediate, untested commits do not show up on master.

-> CS  Thanks, Jonas for your hard work on this.  I think this will be a great step forward.

Carl




Reply | Threaded
Open this post in threaded view
|

Re: Obstacles for using GitLab CI

David Kastrup
Carl Sorensen <[hidden email]> writes:

> On 5/14/20, 2:31 AM, "lilypond-devel on behalf of Jonas Hahnfeld"
> <lilypond-devel-bounces+c_sorensen=[hidden email] on behalf of
> [hidden email]> wrote:
>
> Am Donnerstag, den 14.05.2020, 08:09 +0100 schrieb James:
>> On a more general question, and not really understanding how this CI
>> workflow will change 'contexturally' what we do, so apologies for if
>> what I am about to say is ignorant, but are we still taking the
>> 'master-must-always-be-good' /
>> 'staging-can-be-bad-because-we-can-force-delete-not-just-revert' approach?
>>
>> Rather than just automate everything and if something brakes we checkin
>> a reversion which then makes the tree not 100% compilable?
>>
>> I've liked that approach we take so far and it always instills a level
>> of confidence about master.
>
> We would keep 'master-must-always-be-good' but without having an
> explicit staging branch that we need to revert. Instead we ask GitLab
> to only add commits to master after they have passed testing. So
> staging + patchy is basically replaced by merge request + GitLab
> verifying the result of CI. My proposal will hopefully clear this up
> with concrete examples.
>
> -> CS I hope we can get back to the previous mode we worked in that
> not only is master always good, but that every commit on master is
> known to work.

Strange quoting style.  At any rate, it's probably worth stating a few
consequences of our old staging/master setup:

Stuff managed to migrate to master when at least _one_ version of Patchy
had checked it to be good in the tested content.

Patchy, however, is set up in a manner where it picks up work not when
staging is ahead of master, but rather when staging is ahead of its last
tested version.

That means that even if the migration to master may proceed with the
vote of one Patchy, _every_ instance of Patchy will look at whether it
is satisfied with the current state in a timely manner.

So the diversity of our Patchy setups may not keep something working
only on some platforms from getting into master, but it will still not
stay under the radar indefinitely.

--
David Kastrup

Reply | Threaded
Open this post in threaded view
|

Re: FW: Obstacles for using GitLab CI

Carl Sorensen
> On 5/14/20, 8:04 AM, "David Kastrup" <[hidden email]> wrote:
>
> Carl Sorensen <[hidden email]> writes:
>
> > On 5/14/20, 2:31 AM, "lilypond-devel on behalf of Jonas Hahnfeld"
> > <lilypond-devel-bounces+c_sorensen=[hidden email] on behalf of
> > [hidden email]> wrote:
> >
> > Am Donnerstag, den 14.05.2020, 08:09 +0100 schrieb James:
> >> On a more general question, and not really understanding how this CI
> >> workflow will change 'contexturally' what we do, so apologies for if
> >> what I am about to say is ignorant, but are we still taking the
> >> 'master-must-always-be-good' /
> >> 'staging-can-be-bad-because-we-can-force-delete-not-just-revert'
> approach?
> >>
> >> Rather than just automate everything and if something brakes we checkin
> >> a reversion which then makes the tree not 100% compilable?
> >>
> >> I've liked that approach we take so far and it always instills a level
> >> of confidence about master.
> >
> > We would keep 'master-must-always-be-good' but without having an
> > explicit staging branch that we need to revert. Instead we ask GitLab
> > to only add commits to master after they have passed testing. So
> > staging + patchy is basically replaced by merge request + GitLab
> > verifying the result of CI. My proposal will hopefully clear this up
> > with concrete examples.
> >
> > -> CS I hope we can get back to the previous mode we worked in that
> > not only is master always good, but that every commit on master is
> > known to work.
>
> Strange quoting style.  At any rate, it's probably worth stating a few
> consequences of our old staging/master setup:


Yes.  I have been using Microsoft Outlook due to our standards at work.
Microsoft has disabled internet quoting style in Outlook 2016 for MacOS.
This alternative was proposed by Werner.  I've finally decided it's more
bother than changing my lilypond-devel subscription to Gmail and using a
different mail client for my LilyPond work.


> Stuff managed to migrate to master when at least _one_ version of Patchy
> had checked it to be good in the tested content.
>
> Patchy, however, is set up in a manner where it picks up work not when
> staging is ahead of master, but rather when staging is ahead of its last
> tested version.
>
> That means that even if the migration to master may proceed with the
> vote of one Patchy, _every_ instance of Patchy will look at whether it
> is satisfied with the current state in a timely manner.
>
> So the diversity of our Patchy setups may not keep something working
> only on some platforms from getting into master, but it will still not
> stay under the radar indefinitely.
>

I think you have not understood the issue with which I am concerned.
Suppose I have a fix for an issue that contains six commits.  The first 3
result in failure to make doc.  The last 3 fix the problem.  So after the
set of 6 commits, everything is good.

Under our past standards, I would be expected to rebase those six commits
into a single commit and push to staging.  Thus, staging builds.

Many of the commits I have been verifying from the last year have not
rebased those six commits into a single commit.  The six individual commits
are pushed to staging, each with its own commit ID.  But since they are all
pushed at one time, Patchy never tests the state after commit 2.

Now, a year later, we're trying to find a regression.  We're using
git-bisect.   git-bisect lands on commit 2 and tries (and fails) to build.
This messes up my git-bisect run, and I have to deal with things manually.

My request is that we have some way of preventing this problem by either
(a) having our CI test each of my six commits and finding out that 3 of
them fail, thus requiring me to rebase before the PR is accepted; or (b)
having an automated merge process that combines my six commits into a
single commit and then tests it with CI to demonstrate that it passes, and
only the combined commit gets sent to master.

Thanks,

Carl
Reply | Threaded
Open this post in threaded view
|

Re: FW: Obstacles for using GitLab CI

David Kastrup
Carl Sorensen <[hidden email]> writes:

>> On 5/14/20, 8:04 AM, "David Kastrup" <[hidden email]> wrote:
>>
>> Patchy, however, is set up in a manner where it picks up work not when
>> staging is ahead of master, but rather when staging is ahead of its last
>> tested version.
>>
>> That means that even if the migration to master may proceed with the
>> vote of one Patchy, _every_ instance of Patchy will look at whether it
>> is satisfied with the current state in a timely manner.
>>
>> So the diversity of our Patchy setups may not keep something working
>> only on some platforms from getting into master, but it will still not
>> stay under the radar indefinitely.
>>
>
> I think you have not understood the issue with which I am concerned.
> Suppose I have a fix for an issue that contains six commits.  The
> first 3 result in failure to make doc.  The last 3 fix the problem.
> So after the set of 6 commits, everything is good.
>
> Under our past standards, I would be expected to rebase those six
> commits into a single commit and push to staging.

Not necessarily.  Particularly work where 95% of the work is done by
script and 5% of manual cleanup, merging both commits is not a good
idea.  I usually push those as a merge.  Doesn't strictly help against
hitting the middle with git bisect, but at least it makes it quite clear
what is happening then.

> Thus, staging builds.
>
> Many of the commits I have been verifying from the last year have not
> rebased those six commits into a single commit.  The six individual
> commits are pushed to staging, each with its own commit ID.  But since
> they are all pushed at one time, Patchy never tests the state after
> commit 2.
>
> Now, a year later, we're trying to find a regression.  We're using
> git-bisect.  git-bisect lands on commit 2 and tries (and fails) to
> build.  This messes up my git-bisect run, and I have to deal with
> things manually.
>
> My request is that we have some way of preventing this problem by
> either (a) having our CI test each of my six commits and finding out
> that 3 of them fail, thus requiring me to rebase before the PR is
> accepted; or (b) having an automated merge process that combines my
> six commits into a single commit and then tests it with CI to
> demonstrate that it passes, and only the combined commit gets sent to
> master.

I think that this involves making decisions that depend on human
discretion.

--
David Kastrup

Reply | Threaded
Open this post in threaded view
|

Re: Obstacles for using GitLab CI

Jonas Hahnfeld
In reply to this post by Carl Sorensen-3
Am Donnerstag, den 14.05.2020, 13:04 +0000 schrieb Carl Sorensen:

>
> On 5/14/20, 2:31 AM, "lilypond-devel on behalf of Jonas Hahnfeld" <
> lilypond-devel-bounces+c_sorensen=[hidden email]
>  on behalf of
> [hidden email]
> > wrote:
>
> Am Donnerstag, den 14.05.2020, 08:09 +0100 schrieb James:
> > On a more general question, and not really understanding how this CI
> > workflow will change 'contexturally' what we do, so apologies for if
> > what I am about to say is ignorant, but are we still taking the
> > 'master-must-always-be-good' /
> > 'staging-can-be-bad-because-we-can-force-delete-not-just-revert' approach?
> >
> > Rather than just automate everything and if something brakes we checkin
> > a reversion which then makes the tree not 100% compilable?
> >
> > I've liked that approach we take so far and it always instills a level
> > of confidence about master.
>
> We would keep 'master-must-always-be-good' but without having an
> explicit staging branch that we need to revert. Instead we ask GitLab
> to only add commits to master after they have passed testing. So
> staging + patchy is basically replaced by merge request + GitLab
> verifying the result of CI. My proposal will hopefully clear this up
> with concrete examples.
>
> -> CS  I hope we can get back to the previous mode we worked in that not only is master always good, but that every commit on master is known to work.
>
> -> CS  As I have been verifying issues fixed for 2.20.1 I have found a number of issues that were resolved with 2, 3, 4, or as many as 7 commits.  As far as I know, we have no testing that indicates each of these commits builds correctly, only that the full set of commits builds correctly.  We made a conscious decision to require only single-commit fixes so that every commit on master would build, and we could use git-bisect effectively to identify regressions.
>
> -> CS  I know that we had some discussion about how it is much more convenient to have multiple small commits for reviewing, and I can see that in nearly every case the multiple commits were aimed at helping reviewers understand the patches, which I think is a great thing.  I'd be happy to keep multiple commits for reviewing, but I believe we need to ensure that every commit on master builds properly to support git-bisect.
>
> -> CS  I hope that we can make our CI/merge process function such that either we run the CI on every commit in a merge request or we rebase/squash/make a single merge commit so that the intermediate, untested commits do not show up on master.
I'm not aiming to do this, in my opinion it's a waste of resources to
run the full pipeline on every single commit. AFAIK GitLab CI doesn't
even support this?
I also oppose to saying that multiple smaller commits are bad and
everything should be squashed; there are enough guides out there
recommending the contrary and I think they are right: Commits are a
trace of logical changes. If a fix requires multiple changes that lend
themselves to splitting, this is favorable to almost any inspection of
code history.
(NB changes for review comments are not "logical" commits for me and
should be folded into the respective "base" commit.)

The ability to bisect is somewhat orthogonal: I agree that every commit
should build in principle, but eventually tooling updates or other
factors can still limit what you're able to compile in a years time.
Automated checks or even manual ones can't catch everything that could
go wrong in the future.

So in total this is a compromise between resources and the potential
benefit. I think testing at the level of merge requests (as we do right
now) is a good trade-off. It also ensure that master is always
compilable, at least in the way testing ensures.
All of this eventually comes down to trusting contributors to split the
changes in a meaningful way...

Jonas

signature.asc (499 bytes) Download Attachment