Skip to main content
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [List Home]
Re: [theia-dev] New feature enabled for CI: Require branches to be up to date before merging
  • From: Rob Moran <Rob.Moran@xxxxxxx>
  • Date: Tue, 28 Mar 2023 10:43:36 +0000
  • Accept-language: en-GB, en-US
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=arm.com; dmarc=pass action=none header.from=arm.com; dkim=pass header.d=arm.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=O7XUEiFB8u6ns5uwCnCHnqamZ2dmitI4TmTUZPWjv0E=; b=dwdm89lo2L7r6SvC1nrfIZsKMOdGxk6YsdMU5efWdcB3QVRRQLe/cU8q2hbOFI2x4D/EqYi4rmgV3ojFqJfqNkff7cUMzh0KZQ2PtSENsN3jDoeFVqtBuPZkDc8f6T8uWRCPX6C/G2BPMG2aEWQMdiHcEfIzbdKsxuihR0YrAtd4N5ghUkiylUkFRsUvOmy89MFlEcBRpQLf9F5gkCrzgo5DVQH+kAcwvU+qqyjkx5PB2eNmUg+1hp5OD/ZL61Km5R5OPBx03MnLfv2nisNIbz6hme91xCXBGq5hqNnBcOYwHDWQBAuyydjobcK6T5CbmyOV3Of1aRtOD4bLTvn+Ag==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=lFffriiVTCHZHgswGF0RQRu2G7ghVbNDtopRAkbbFCXU7SOd4D9RHxtEcH/K4PNLaJ5xKP7SHZXicB2A7/NIhMhGJhr4aB8bEE0CYnBekY0zkKkbpAbqBWj1KFBJANTPkck8ZXCA+hmTbqB1TwKq8DsfeE5qYJtjRxuwaf6zyBWSxkkSef6un7U+WpkHxxJYxKPp1Dm2KJkDb2dilBkUE2S/JFZYd/hB3CdlogvSBAyRrOcZJ1ktSmGYkrGBbOrXLsRZJQbOSFcizz31nvLHeT0EevM5/MvraLXd2+uoMR+V5QsvjFmH97dBNejYcr6lN8Ow3VkJPc8RwjvjL+jULQ==
  • Delivered-to: theia-dev@xxxxxxxxxxx
  • List-archive: <https://www.eclipse.org/mailman/private/theia-dev/>
  • List-help: <mailto:theia-dev-request@eclipse.org?subject=help>
  • List-subscribe: <https://www.eclipse.org/mailman/listinfo/theia-dev>, <mailto:theia-dev-request@eclipse.org?subject=subscribe>
  • List-unsubscribe: <https://www.eclipse.org/mailman/options/theia-dev>, <mailto:theia-dev-request@eclipse.org?subject=unsubscribe>
  • Msip_labels:
  • Nodisclaimer: true
  • Original-authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=arm.com;
  • Thread-index: AQHZXY7F2oE1JyYYQ0GvJQ3HifOszq8IbqqAgAABt+uABiillYAAFJPAgAFalOM=
  • Thread-topic: [theia-dev] New feature enabled for CI: Require branches to be up to date before merging
Hi all,

this is really good feedback and perhaps suggests our experiment isn't the right way forward.

We could use the review checklist questions in the PR template as a looser way to make sure people are considering the impact of their changes?

Kind regards,

Rob

From: theia-dev <theia-dev-bounces@xxxxxxxxxxx> on behalf of Colin Grant via theia-dev <theia-dev@xxxxxxxxxxx>
Sent: 27 March 2023 15:48
To: theia developer discussions <theia-dev@xxxxxxxxxxx>
Cc: Colin Grant <colin.grant@xxxxxxxxxxxx>
Subject: Re: [theia-dev] New feature enabled for CI: Require branches to be up to date before merging
 

Hello all,

 

I would add to Vince’s points that CI is still not so stable that rebasing is always trivial: on a couple of my recent PR’s both the Playwright tests and the browser test suite have produced false positives that have disappeared with a rerun. More rebases means more opportunities for such false positives, which are a minor annoyance in a committer’s PR’s, since we can easily rerun CI, but a greater annoyance in a contributor’s PR’s, since a committer has to rerun them, and the contributor may not understand why they’ve occurred, let alone reoccurred.

 

Best,

Colin

 

From: theia-dev <theia-dev-bounces@xxxxxxxxxxx> On Behalf Of Vincent Fugnitto via theia-dev
Sent: Monday, March 27, 2023 6:55 AM
To: theia developer discussions <theia-dev@xxxxxxxxxxx>
Cc: Vincent Fugnitto <vincent.fugnitto@xxxxxxxxxxxx>
Subject: Re: [theia-dev] New feature enabled for CI: Require branches to be up to date before merging

 

Hi everyone,

 

I believe there are some significant drawbacks with the new "require branches to be up to date before merging" option during CI as it makes working in open-source (especially for non-committers) more difficult.

 

The main reasons are:

  • The process adds friction for non-committers when instead we want to encourage them to contribute
  • There does not seem to be the automatic "update" button during CI from forks unless explicity set by each contributor
  • Asking contributors to constantly rebase since master was updated adds friction, and it becomes increasingly difficult to get something merged from different time zones (ex: China) as master is a moving target

I think we may want to revisit the option, especially since we haven't really encountered an issue with an outdated pull-request often, and the drawbacks I've mentioned.

I can add a note in the dev meeting if needed.

 

Thanks!

Vince

From: theia-dev <theia-dev-bounces@xxxxxxxxxxx> on behalf of Marc Dumais via theia-dev <theia-dev@xxxxxxxxxxx>
Sent: March 23, 2023 11:00 AM
To: theia developer discussions <theia-dev@xxxxxxxxxxx>; Thomas Mäder <t.s.maeder@xxxxxxxxx>
Cc: Marc Dumais <marc.dumais@xxxxxxxxxxxx>
Subject: Re: [theia-dev] New feature enabled for CI: Require branches to be up to date before merging

 

Hi Thomas,

 

I had not thought about that, but it does make sense. In fact, I do see it happening on your PR about enabling context isolation, that originates from your fork, where presumably you have not granted permission for maintainers to edit. 

 

That, coupled with the need for one of our project committers (user with write access to our repo) to push the "Approve and run" CI button, post-branch-update, might make for a bit of back-and-forth between us and external (non-committer) contributors. 

 

One thing we could do to mitigate this is to update our PR template to encourage external contributors to grant us "allow edits from maintainers" on their PR? Maybe we can do this after the trial period of one month is over?

 

Regards,

Marc

From: theia-dev <theia-dev-bounces@xxxxxxxxxxx> on behalf of Thomas Mäder <t.s.maeder@xxxxxxxxx>
Sent: Thursday, March 23, 2023 10:36 AM
To: theia developer discussions <theia-dev@xxxxxxxxxxx>
Subject: Re: [theia-dev] New feature enabled for CI: Require branches to be up to date before merging

 

Very cool, thank you. 

I guess you need write access to the branch to be merged in order to see the "Update branch" button, right? Because it's your own branch or the contributor checked "allow edits from maintainers"?

 

/Thomas

 

 

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Back to the top