Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove approve-and-merge #32972

Merged
merged 4 commits into from
Dec 1, 2023
Merged

Remove approve-and-merge #32972

merged 4 commits into from
Dec 1, 2023

Conversation

bnchrch
Copy link
Contributor

@bnchrch bnchrch commented Nov 30, 2023

Overview

TLDR: We saying goodbye to our dear friend /approve-and-merge today.

After a seed was planted by @postamar (https://airbytehq-team.slack.com/archives/C02UF50V9HA/p1696626683363899) and a longer discussion with @git-phu @bgroff @jdpgrailsdev @colesnodgrass @stephane-airbyte and @davinchia its been determined that at this point the /approve-and-merge command has started to hurt more than its helped.

Why?
No doubt its be a useful tool and a steadfast companion.

However as of late its been responsible for a number of issues

  1. The merging of unformatted or linted code
  2. Failing Unit Tests on master
  3. Failing Integration Tests on master

And the biggest problem by far. Its gotten us all comfortable with subpar CI. Unnecessary tests running on docs changes, Flaky unit tests, and in some cases an excessively slow test suite.

Part of the change is to be preventative, but an equal part of removing this command is help us all notice where we can and should improve.

Why now?
Over the last 2 months we've

  1. Fixed a number of broken and flaky tests
  2. Updated Github Actions to allow for required checks to be skipped. (i.e. markdown only changes should not trigger the full test suite)
  3. Sped up our entire test suite (Gradle tests, formatting, Connector Acceptance Tests)

And we believe we're now in a much better place where the time to run CI is much more tolerable.

What do I do now?
All EM's and many Senior Engineers at Airbyte have admin merge privileges.

If there is an urgent PR ping any of them.

If its the middle of the night and youre OC, look at the secondary rotation, they are Admins, page them.

If the integration test is broken, fix it.

and if the test is a known issue and too much effort to fix inside your task ping your EM, get it merged and get the space to fix the underlying issue (or remove it).

What if I think this is stupid?
Let us know! We're going to reevaluate this decision inside of the next two weeks.

Copy link

vercel bot commented Nov 30, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
airbyte-docs ⬜️ Ignored (Inspect) Visit Preview Nov 30, 2023 7:29pm

Copy link
Contributor

@postamar postamar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Obviously this will create more work for Connector Ops and Prod Eng, not to mention admin-privilege-havers, still, I agree that it's worth doing now and the connectors CI is sufficiently mature.

@bnchrch bnchrch marked this pull request as ready for review November 30, 2023 17:42
@bnchrch bnchrch merged commit 1874a4f into master Dec 1, 2023
13 checks passed
@bnchrch bnchrch deleted the bnchrch/remove-approve-and-merge branch December 1, 2023 01:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants