create auto_merge package #38019

alafanechere · 2024-05-07T11:34:52Z

What

Relates to https://github.com/airbytehq/airbyte-internal-issues/issues/7554
Introduce a new internal package to automatically merge pull requests on the airbyte repo

How

Read the README.md please 🙏

User Impact

None, the tool is not used in a GHA workflow yet

vercel · 2024-05-07T11:34:56Z

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		May 13, 2024 10:13am

alafanechere · 2024-05-07T11:35:10Z

declare new auto_merge scheduled workflow #38022
airbyte-ci: add auto_merge as an internal package #38020
create auto_merge package #38019 👈
master

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @alafanechere and the rest of your teammates on Graphite

natikgadzhi

LGTM, reviewing to unblock.

A few nits that I would love you to work through, but I assume there's a stack of PRs where you could clean those up.

natikgadzhi · 2024-05-10T13:37:38Z

airbyte-ci/README.md

@@ -15,3 +15,4 @@ The installation instructions for the `airbyte-ci` CLI tool cal be found here
 | [`connectors_qa`](connectors/connectors_qa/)       | A tool to verify connectors have sounds assets and metadata.                                                                  |
 | [`metadata_service`](connectors/metadata_service/) | Tools to generate connector metadata and registry.                                                                            |
 | [`pipelines`](connectors/pipelines/)               | Airbyte CI pipelines, including formatting, linting, building, testing connectors, etc. Connector acceptance tests live here. |
+| [`auto_merge`](connectors/auto_merge/)             | A tool to automatically merge connector pull requests.                                                                        |


Thank you for adding it to the index!

Nit: can you also add the live testing tool to this index in here or separate quick PR?

natikgadzhi · 2024-05-10T13:38:10Z

airbyte-ci/connectors/auto_merge/README.md

@@ -0,0 +1,33 @@
+# `auto_merge`
+
+This python package is made to merge pull requests automatically on the Airbyte Repo. It is used in the [following workflow](TBD).


Nit: don't forget to update this link (I'm assuming it's in the next PR?)

natikgadzhi · 2024-05-10T13:38:37Z

airbyte-ci/connectors/auto_merge/README.md

+- All the required checks have passed
+
+
+## Install and usage


Nit: prettier -w this.

natikgadzhi · 2024-05-10T13:39:44Z

airbyte-ci/connectors/auto_merge/pyproject.toml

+
+[tool.airbyte_ci]
+optional_poetry_groups = ["dev"]
+poe_tasks = ["type_check", "lint",]


Suggested change

poe_tasks = ["type_check", "lint",]

poe_tasks = ["type_check", "lint",]

natikgadzhi · 2024-05-10T13:40:26Z

airbyte-ci/connectors/auto_merge/src/auto_merge/consts.py

@@ -0,0 +1,12 @@
+# Copyright (c) 2024 Airbyte, Inc., all rights reserved.
+
+from __future__ import annotations


Suggested change

from __future__ import annotations

I think from __future__ import annotations is not needed in Python 3.10?

natikgadzhi · 2024-05-10T13:40:55Z

airbyte-ci/connectors/auto_merge/src/auto_merge/consts.py

+AUTO_MERGE_LABEL = "auto-merge"
+BASE_BRANCH = "master"
+CONNECTOR_PATH_PREFIXES = {
+    "airbyte-integrations/connectors/",


Nit: this one has trailing /, the other two do not. I don't think this is intentional?

natikgadzhi · 2024-05-10T13:42:20Z

airbyte-ci/connectors/auto_merge/src/auto_merge/main.py

+
+
+@contextmanager
+def github_client() -> Iterator[Github]:


natikgadzhi · 2024-05-10T13:43:25Z

airbyte-ci/connectors/auto_merge/src/auto_merge/main.py

+            client.close()
+
+
+def check_if_modifies_connectors_only(modified_files: Iterable[GithubFile]) -> bool:


I like this. Ideally, we'd like to verify that only files in a single connector are modified, but honestly I think this would be too granular to focus on here.

natikgadzhi · 2024-05-10T13:44:02Z

airbyte-ci/connectors/auto_merge/src/auto_merge/main.py

+        bool: True if the head commit passes all required checks, False otherwise
+    """
+    successful_status_contexts = [commit_status.context for commit_status in head_commit.get_statuses() if commit_status.state == "success"]
+    successful_check_runs = [check_run.name for check_run in head_commit.get_check_runs() if check_run.conclusion == "success"]


Why, TIL you can get check status in Python code! This is very neat.

natikgadzhi · 2024-05-10T13:45:47Z

airbyte-ci/connectors/auto_merge/src/auto_merge/main.py

+    if not has_auto_merge_label:
+        logging.info(f"PR {pr.number} does not have the {AUTO_MERGE_LABEL} label")
+        return False
+    targets_main_branch = pr.base.ref == BASE_BRANCH


Nit: you're declaring variables that are only used once on the very next line — let's just use the if statement directly?

I.e. if not pr.base.ref == BASE_BRANCH:. I don't think you will loose any readability with that.

clnoll

Nice @alafanechere! Is the idea that this will run nightly eventually? Should we send the summary to Slack?

clnoll · 2024-05-10T14:26:28Z

airbyte-ci/connectors/auto_merge/src/auto_merge/env.py

+import os
+
+GITHUB_TOKEN = os.environ["GITHUB_TOKEN"]
+PRODUCTION = os.environ.get("AUTO_MERGE_PRODUCTION", "false").lower() == "true"


Not required for this PR, but we have various ways that we define truthy in airbyte-ci. At some point we should probably try to make them consistent.

bnchrch

This looks great! I dont want to get in the way so im approving now.

The only thing I'd strongly encourage is

Tell users more about the auto merge label, what it is, where it comes from
Consider containing our "What is a conenctor?" logic to our connector ops library.

Future footage of you merging auto merge 👇

bnchrch · 2024-05-10T14:21:50Z

airbyte-ci/connectors/auto_merge/README.md

+This python package is made to merge pull requests automatically on the Airbyte Repo. It is used in the [following workflow](TBD).
+
+A pull request is currently considered as auto-mergeable if:
+- It has the `auto-merge` label


❓ Is this a github label? or a metadata field?

bnchrch · 2024-05-10T14:22:46Z

airbyte-ci/connectors/auto_merge/README.md

@@ -0,0 +1,33 @@
+# `auto_merge`


💡 There could be a section on "Purpose" e.g. explain why we have automerge and that the goal is to merge connectors with as little manual airbyte intervention as possible

bnchrch · 2024-05-10T14:23:21Z

airbyte-ci/connectors/auto_merge/src/auto_merge/env.py

+import os
+
+GITHUB_TOKEN = os.environ["GITHUB_TOKEN"]
+PRODUCTION = os.environ.get("AUTO_MERGE_PRODUCTION", "false").lower() == "true"


bnchrch · 2024-05-10T14:23:53Z

airbyte-ci/connectors/auto_merge/src/auto_merge/main.py

+logging.getLogger().setLevel(logging.INFO)
+
+
+@contextmanager


💎 Oooh gettting fancy I see

bnchrch · 2024-05-10T14:25:24Z

airbyte-ci/connectors/auto_merge/src/auto_merge/main.py

+        bool: True if all modified files are in CONNECTOR_PATH_PREFIXES, False otherwise
+    """
+    for file in modified_files:
+        if not any(file.filename.startswith(prefix) for prefix in CONNECTOR_PATH_PREFIXES):


❓ Didn't we have this same logic in the connector ops package? If so should we introduce that as dependency so if we change where connectors are located we dont have to update so many places?

bnchrch · 2024-05-10T14:41:04Z

airbyte-ci/connectors/auto_merge/src/auto_merge/main.py

+    return required_checks.issubset(successful_contexts)
+
+
+def check_if_pr_is_auto_mergeable(head_commit: GithubCommit, pr: PullRequest, required_checks: set[str]) -> bool:


💡 Ok so here is the most important logic in the system. The business logic.

Its important that its

Readable

Easily extendable

Contained to as few locations (files) as possible

I think youve achieved that so this is suggestion is very optional:

What if we used a validator pattern

def _validate_has_auto_merge_label((head_commit: GithubCommit, pr: PullRequest, required_checks: set[str]) -> bool, Optional[str]: has_auto_merge_label = any(label.name == AUTO_MERGE_LABEL for label in pr.labels) if not has_auto_merge_label: return False, f"PR {pr.number} does not have the {AUTO_MERGE_LABEL} label" return True, None def check_if_pr_is_auto_mergeable(head_commit: GithubCommit, pr: PullRequest, required_checks: set[str]) -> bool: validators = [ _validate_has_auto_merge_label, # ... ] for pr_validator in validators: is_valid, error = pr_validator(head_commit, pr, required_checks) if not is_valid logging.info(error) return False

(this is an example and could be improved (Named ValidationArgument|Result type, standard error messaging, etc) but the ideas there.)

Why this way?

It lets us conditionally add/remove validations for a connector more easily. You can imagine that critical connectors or third party connectors might want to add or skip certain validations

Validation logic is easier to test

imo easier to skim and understand what validations run when/under what conditions

Anyway again very optional

bnchrch · 2024-05-10T14:43:16Z

airbyte-ci/connectors/auto_merge/src/auto_merge/main.py

+        logging.info(f"PR {pr.number} does not target {BASE_BRANCH}")
+        return False
+    touches_connectors_only = check_if_modifies_connectors_only(pr.get_files())
+    if not touches_connectors_only:


💅 Can we put some newlines space between validations?

bnchrch · 2024-05-10T14:43:29Z

airbyte-ci/connectors/auto_merge/src/auto_merge/main.py

+    logging.info(f"Processing PR {pr.number}")
+    head_commit = repo.get_commit(sha=pr.head.sha)
+    if check_if_pr_is_auto_mergeable(head_commit, pr, required_passing_contexts):
+        if not dry_run:


bnchrch · 2024-05-10T14:43:55Z

airbyte-ci/connectors/auto_merge/src/auto_merge/main.py

+        time.sleep(github_client.rate_limiting_resettime - time.time())
+
+
+def generate_job_summary_as_markdown(merged_prs: list[PullRequest]) -> str:


💅 Could move to a helper file

bleonard · 2024-05-11T02:14:39Z

The main thing I'm curious about: if it takes a label anyway that suggests these PRs are auto-created with a nuance or someone clicks the label button. If that's the case, why not use the auto-merge feature of github?
https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/incorporating-changes-from-a-pull-request/automatically-merging-a-pull-request

alafanechere · 2024-05-13T08:15:07Z

The main thing I'm curious about: if it takes a label anyway that suggests these PRs are auto-created with a nuance or someone clicks the label button. If that's the case, why not use the auto-merge feature of github? https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/incorporating-changes-from-a-pull-request/automatically-merging-a-pull-request

@bleonard I think the main nuance here is that this package gives us more flexibility as it can override the branch protection rules the normal "auto merge" GH feature follows.
For instance, it's currently required for any PR to get a review from an org member. With this package we can implement a bit more complex and flexible rules compared to Github auto merge, and decide to make the package merge the PR without review if it meets a certain set of criteria. I also think it's sane to keep the current branch protection rules we have for all PRs.

alafanechere · 2024-05-13T09:52:15Z

Nice @alafanechere! Is the idea that this will run nightly eventually? Should we send the summary to Slack?

Yes this will run on a nightly / weekly basis. I think we could send the summary to slack. Might a logic to add to the GHA workflow and not the python package itself.

bleonard · 2024-05-13T15:47:14Z

For instance, it's currently required for any PR to get a review from an org member. With this package we can implement a bit more complex and flexible rules compared to Github auto merge

The flexibility makes sense for sure. It can't hurt and the footprint is small. I was expecting, and still assume, this flexibility should be built into the actual tests that get run on the connector. I think this is the case now that long-tail connectors aren't requiringt hat review. Or we could make it so. The we'd know from metadata which connectors gets the full CAT tests (because we have sandboxes) and which ones don't. It's useful for "green" to mean something, basically. So I would assume most of the logic is in the tests that get run not in the merging tool.

alafanechere force-pushed the augustin/05-06-create_auto_merge_package branch from 2f072e3 to 4f8ed1b Compare May 7, 2024 11:38

alafanechere mentioned this pull request May 7, 2024

airbyte-ci: add auto_merge as an internal package #38020

Merged

alafanechere marked this pull request as ready for review May 7, 2024 11:40

alafanechere requested review from a team and bleonard May 7, 2024 11:40

alafanechere mentioned this pull request May 7, 2024

declare new auto_merge scheduled workflow #38022

Merged

alafanechere requested review from bnchrch and clnoll May 10, 2024 08:13

natikgadzhi approved these changes May 10, 2024

View reviewed changes

clnoll approved these changes May 10, 2024

View reviewed changes

bnchrch approved these changes May 10, 2024

View reviewed changes

alafanechere added 2 commits May 13, 2024 11:52

create auto_merge package

cb98fa4

review suggestions

da21c54

alafanechere force-pushed the augustin/05-06-create_auto_merge_package branch from 4f8ed1b to da21c54 Compare May 13, 2024 09:55

airbyte-ci: add auto_merge as an internal package (#38020)

51c739d

alafanechere enabled auto-merge (squash) May 13, 2024 10:13

alafanechere merged commit 9179ec5 into master May 13, 2024
34 checks passed

alafanechere deleted the augustin/05-06-create_auto_merge_package branch May 13, 2024 10:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

create auto_merge package #38019

create auto_merge package #38019

alafanechere commented May 7, 2024 •

edited

Loading

vercel bot commented May 7, 2024 •

edited

Loading

alafanechere commented May 7, 2024 •

edited

Loading

natikgadzhi left a comment

natikgadzhi May 10, 2024

natikgadzhi May 10, 2024

natikgadzhi May 10, 2024

natikgadzhi May 10, 2024

natikgadzhi May 10, 2024

natikgadzhi May 10, 2024

natikgadzhi May 10, 2024

natikgadzhi May 10, 2024

natikgadzhi May 10, 2024

natikgadzhi May 10, 2024

clnoll left a comment

clnoll May 10, 2024

bnchrch left a comment

bnchrch May 10, 2024

bnchrch May 10, 2024

bnchrch May 10, 2024

bnchrch May 10, 2024

bnchrch May 10, 2024

bnchrch May 10, 2024

bnchrch May 10, 2024

bnchrch May 10, 2024

bnchrch May 10, 2024

bleonard commented May 11, 2024

alafanechere commented May 13, 2024

alafanechere commented May 13, 2024

bleonard commented May 13, 2024 •

edited

Loading

		@@ -0,0 +1,33 @@
		# `auto_merge`

		This python package is made to merge pull requests automatically on the Airbyte Repo. It is used in the [following workflow](TBD).

	poe_tasks = ["type_check", "lint",]
	poe_tasks = ["type_check", "lint",]

		@@ -0,0 +1,12 @@
		# Copyright (c) 2024 Airbyte, Inc., all rights reserved.

		from __future__ import annotations

		client.close()


		def check_if_modifies_connectors_only(modified_files: Iterable[GithubFile]) -> bool:

		return required_checks.issubset(successful_contexts)


		def check_if_pr_is_auto_mergeable(head_commit: GithubCommit, pr: PullRequest, required_checks: set[str]) -> bool:

		time.sleep(github_client.rate_limiting_resettime - time.time())


		def generate_job_summary_as_markdown(merged_prs: list[PullRequest]) -> str:

		logging.getLogger().setLevel(logging.INFO)


		@contextmanager

create auto_merge package #38019

create auto_merge package #38019

Conversation

alafanechere commented May 7, 2024 • edited Loading

What

How

User Impact

vercel bot commented May 7, 2024 • edited Loading

alafanechere commented May 7, 2024 • edited Loading

natikgadzhi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clnoll left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnchrch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bleonard commented May 11, 2024

alafanechere commented May 13, 2024

alafanechere commented May 13, 2024

bleonard commented May 13, 2024 • edited Loading

alafanechere commented May 7, 2024 •

edited

Loading

vercel bot commented May 7, 2024 •

edited

Loading

alafanechere commented May 7, 2024 •

edited

Loading

bleonard commented May 13, 2024 •

edited

Loading