-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
[airbyte-ci/auto-merge] - Updates PR pipeline to bypass CI checks for promoted release candidate PRs #49827
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pnilan this looks good to me, just a couple of very minor comments. I'll leave the final approval to @alafanechere.
@@ -753,7 +753,7 @@ async def run_connector_promote_pipeline(context: PublishConnectorContext, semap | |||
all_modified_files.update( | |||
await add_changelog_entry.export_modified_files(context.connector.local_connector_documentation_directory) | |||
) | |||
post_changelog_pr_update = CreateOrUpdatePullRequest(context, skip_ci=False, labels=["auto-merge"]) | |||
post_changelog_pr_update = CreateOrUpdatePullRequest(context, skip_ci=True, labels=["auto-merge", "auto-merge/promoted-rc"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can "auto-merge"
and "auto-merge/promoted-rc"
be pulled in from the constants file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could ideally add the auto_merge
packages as a dependency of pipelines
and import constant from it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to add automerge as a dependency.
@@ -48,12 +48,13 @@ def head_commit_passes_all_required_checks( | |||
return True, None | |||
|
|||
|
|||
# A PR is considered auto-mergeable if: | |||
# A standard PR is considered auto-mergeable if: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: since we don't define what "standard" means here, I'd prefer to leave this comment as-is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reverted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @pnilan . The implementation looks correct. Please consider my suggestion which I think would help in the maintenance of the package in case other use cases like this arise in the future. And can you please bump the auto_merge
package version in pyproject.toml
?
# Let's declare faster checks first as the check_if_pr_is_auto_mergeable function fails fast. | ||
DEFAULT_ENABLED_VALIDATORS = [has_auto_merge_label, targets_main_branch, only_modifies_connectors, head_commit_passes_all_required_checks] | ||
PROMOTED_RC_PR_VALIDATORS = [has_auto_merge_label, targets_main_branch, only_modifies_connectors] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I'd reverse the logic.
The default validators would have less requirements than a second one which would require the head commit to pass.
DEFAULT_ENABLED_VALIDATORS = {has_auto_merge_label, targets_main_branch, only_modifies_connectors}
PASSING_CHECKS_VALIDATORS = DEFAULT_ENABLED_VALIDATORS + { "head_commit_passes_all_required_checks"}
It would define an implicit logic of strictness elevation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree -- updated.
AUTO_MERGE_LABEL = "auto-merge" | ||
AUTO_MERGE_PROMOTED_RC_LABEL = "auto-merge/promoted-rc" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest removing the auto-merge/
prefix and defining a mapping of labels to validator groups. This would allow to easily expand your implementation to other use cases.
E.g : A PR has the auto-merge
label AND promoted-rc
-> use a specific set of validators. We could then add different combination with only a single change to make to a mapping.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated this by adding a get_pr_validators
function and a label -> validator mapping dictionary. Take a look and let me know what you think.
@@ -753,7 +753,7 @@ async def run_connector_promote_pipeline(context: PublishConnectorContext, semap | |||
all_modified_files.update( | |||
await add_changelog_entry.export_modified_files(context.connector.local_connector_documentation_directory) | |||
) | |||
post_changelog_pr_update = CreateOrUpdatePullRequest(context, skip_ci=False, labels=["auto-merge"]) | |||
post_changelog_pr_update = CreateOrUpdatePullRequest(context, skip_ci=True, labels=["auto-merge", "auto-merge/promoted-rc"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could ideally add the auto_merge
packages as a dependency of pipelines
and import constant from it.
c4e0dd9
to
7b0d997
Compare
@@ -115,7 +115,7 @@ async def get_file_contents(container: Container, path: str) -> Optional[str]: | |||
def catch_exec_error_group() -> Generator: | |||
try: | |||
yield | |||
except anyio.ExceptionGroup as eg: | |||
except ExceptionGroup as eg: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the inclusion of auto-merge
as a library, I needed to update the anyio
version to at least 4.0.0, which deprecated anyio.ExceptionGroup
in favor of the built-in ExceptionGroup
. See here: https://anyio.readthedocs.io/en/stable/migration.html#task-groups-now-wrap-single-exceptions-in-groups
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
# Let's declare faster checks first as the check_if_pr_is_auto_mergeable function fails fast. | ||
VALIDATOR_MAPPING: dict[str, list[Callable]] = { | ||
AUTO_MERGE_LABEL: [has_auto_merge_label, targets_main_branch, only_modifies_connectors, head_commit_passes_all_required_checks], | ||
AUTO_MERGE_BYPASS_CI_CHECKS_LABEL: [has_auto_merge_bypass_ci_checks_label, targets_main_branch, only_modifies_connectors], | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit for readability
# Let's declare faster checks first as the check_if_pr_is_auto_mergeable function fails fast. | |
VALIDATOR_MAPPING: dict[str, list[Callable]] = { | |
AUTO_MERGE_LABEL: [has_auto_merge_label, targets_main_branch, only_modifies_connectors, head_commit_passes_all_required_checks], | |
AUTO_MERGE_BYPASS_CI_CHECKS_LABEL: [has_auto_merge_bypass_ci_checks_label, targets_main_branch, only_modifies_connectors], | |
} | |
COMMON_VALIDATORS = { | |
has_auto_merge_label, | |
targets_main_branch, | |
only_modifies_connectors, | |
} | |
# Let's declare faster checks first as the check_if_pr_is_auto_mergeable function fails fast. | |
VALIDATOR_MAPPING: dict[str, set[Callable]] = { | |
AUTO_MERGE_LABEL: COMMON_VALIDATORS + {head_commit_passes_all_required_checks}, | |
AUTO_MERGE_BYPASS_CI_CHECKS_LABEL: COMMON_VALIDATORS, | |
} |
What
auto-merge/bypass-checks
label to publishing pipeline when publishing with--promote-release-candidate
flag.auto-merge
workflow includes label mapping to required validator methods. If newautom-merge/byapss-checks
label included in a given PR, auto-merge will run all validator methods except for the CI checks one.How
auto-merge/bypass-ci-checks
label added to PR when runningpublish
pipeline with--promote-release-candidate
flag. Also newpromoted-rc
label added for human filtering of PRsget_pr_validators
method checks the PR's labels against the newVALIDATOR_MAPPING
and returns mapped pr validator methods.Review guide
Can this PR be safely reverted and rolled back?