From 659f9a0839743ba905a4055c457518ec93662810 Mon Sep 17 00:00:00 2001 From: jlowin Date: Thu, 30 Jun 2016 19:54:27 -0400 Subject: [PATCH 1/7] [AIRFLOW-187] Move "Close XXX" message to end of squash commit --- dev/airflow-pr | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/dev/airflow-pr b/dev/airflow-pr index 937ce280f7ae2..698543f91abad 100755 --- a/dev/airflow-pr +++ b/dev/airflow-pr @@ -229,12 +229,6 @@ def merge_pr(pr_num, target_ref, title, body, pr_repo_desc, local): 'Committer: %s <%s>' % (committer_name, committer_email)) merge_message_flags.extend(["-m", message]) - # The string "Closes #%s" string is required for GitHub to correctly - # close the PR. GitHub will mark the PR as closed, not merged - close_msg = "closes #{}".format(pr_num) - merge_message_flags.extend(["-m", "{} from {}".format( - close_msg.capitalize(), pr_repo_desc)]) - # -- add individual commit messages to squash commit msg = click.style( 'Would you like to include the individual commit messages ' @@ -243,6 +237,13 @@ def merge_pr(pr_num, target_ref, title, body, pr_repo_desc, local): if pr_commits and click.confirm(msg, default=True, prompt_suffix=''): for commit in pr_commits: merge_message_flags.extend(['-m', commit['commit']['message']]) + + # The string "Closes #%s" string is required for GitHub to correctly + # close the PR. GitHub will mark the PR as closed, not merged + close_msg = "closes #{}".format(pr_num) + merge_message_flags.extend(["-m", "{} from {}".format( + close_msg.capitalize(), pr_repo_desc)]) + else: # This will mark the PR as merged merge_message_flags.extend([ From 656f3c60016d0ccd21b46afcb8180edfa9b9a1df Mon Sep 17 00:00:00 2001 From: jlowin Date: Thu, 30 Jun 2016 19:54:48 -0400 Subject: [PATCH 2/7] [AIRFLOW-187] Fix typo in argument name --- dev/airflow-pr | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dev/airflow-pr b/dev/airflow-pr index 698543f91abad..e78485abafb8f 100755 --- a/dev/airflow-pr +++ b/dev/airflow-pr @@ -766,7 +766,7 @@ def main(pr_num, local=False): if click.confirm('Would you like to resolve another JIRA issue?'): resolve_jira_issues_loop( comment=jira_comment, - merged_branches=merged_refs) + merge_branches=merged_refs) @click.group() From bfd2cfc29575280892e5788a49318a8f2eb2d1f3 Mon Sep 17 00:00:00 2001 From: jlowin Date: Thu, 30 Jun 2016 19:55:02 -0400 Subject: [PATCH 3/7] [AIRFLOW-187] Improve prompt styling --- dev/airflow-pr | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/dev/airflow-pr b/dev/airflow-pr index e78485abafb8f..3650dcbf783f4 100755 --- a/dev/airflow-pr +++ b/dev/airflow-pr @@ -295,8 +295,9 @@ def merge_pr(pr_num, target_ref, title, body, pr_repo_desc, local): return else: continue_maybe( - 'The local merge is complete (%s). Push to Apache (%s)?' % ( - target_branch_name, APACHE_REMOTE_NAME)) + 'The local merge is complete ({}). '.format(target_branch_name) + + click.style( + 'Push to Apache ({})?'.format(APACHE_REMOTE_NAME), 'red')) try: run_cmd('git push %s %s:%s' % (APACHE_REMOTE_NAME, target_branch_name, target_ref)) @@ -763,7 +764,9 @@ def main(pr_num, local=False): comment=jira_comment, merge_branches=merged_refs) - if click.confirm('Would you like to resolve another JIRA issue?'): + if click.confirm(click.style( + 'Would you like to resolve another JIRA issue?', + fg='blue', bold=True)): resolve_jira_issues_loop( comment=jira_comment, merge_branches=merged_refs) From f3a05d4733acb2634ab49fa6bc1d83cf55289569 Mon Sep 17 00:00:00 2001 From: jlowin Date: Thu, 30 Jun 2016 20:18:04 -0400 Subject: [PATCH 4/7] [AIRFLOW-302] Improve default squash commit message Previously, we always used the PR title as the squash commit subject. Now, if the squash contains only one commit, then we use the commit message for the squash commit message. If the squash contains more than one commit, we default to the old behavior (using the PR title). We still ask if the user wants to include the PR body, but we only ask if they want to include the individual commits if there was more than one. --- dev/airflow-pr | 103 ++++++++++++++++++++++++++++++------------------- 1 file changed, 63 insertions(+), 40 deletions(-) diff --git a/dev/airflow-pr b/dev/airflow-pr index 3650dcbf783f4..e3cb3d3943353 100755 --- a/dev/airflow-pr +++ b/dev/airflow-pr @@ -184,41 +184,32 @@ def merge_pr(pr_num, target_ref, title, body, pr_repo_desc, local): merge_message_flags = [] if squash: - commit_authors = run_cmd( - ['git', 'log', 'HEAD..{}'.format(pr_branch_name), - '--pretty=format:%an <%ae>'], echo_cmd=False).split("\n") - distinct_authors = sorted( - set(commit_authors), - key=lambda x: commit_authors.count(x), - reverse=True) - primary_author = click.prompt( - click.style( - 'Enter the primary author in the format of \"name \"', - fg='blue', bold=True), - default=distinct_authors[0]) - if primary_author == "": - primary_author = distinct_authors[0] - - commits = run_cmd( - ['git', 'log', 'HEAD..%s' % pr_branch_name, - '--pretty=format:%h [%an] %s'], echo_cmd=False).split("\n\n") - - # -- set authors and add authors to commit message - authors = "\n".join(["Author: %s" % a for a in distinct_authors]) - merge_message_flags.append('--author="{}"'.format(primary_author)) - - # -- Add PR to commit message - merge_message_flags.extend(["-m", title]) - msg = click.style( - 'Would you like to include the PR body in the squash ' - 'commit message?', - fg='blue', bold=True) - if body and click.confirm(msg, default=False, prompt_suffix=''): - # We remove @ symbols from the body to avoid triggering e-mails - # to people every time someone creates a public fork of Airflow. - merge_message_flags += ["-m", body.replace("@", "")] - + # -- create commit message subject + # if there is only one commit, take the squash commit message from it + if len(pr_commits) == 1: + click.echo(click.style( + 'This squash contains only one commit, so we will use its\n' + 'commit message for the squash commit message. You will have\n' + 'an opportunity to edit it later.', bold=True)) + commit_message = pr_commits[0]['commit']['message'] + merge_message_flags.extend(["-m", commit_message]) + # if there is are multiple commits, take the squash commit message from + # the PR title + else: + click.echo(click.style( + 'This squash contains more than one commit, so we will use\n' + 'the PR title as the squash commit subject. You will have an\n' + 'opportunity to edit it later.', bold=True)) + merge_message_flags.extend(["-m", title]) + + commit_subject = merge_message_flags[-1].split('\n')[0] + if not re.findall("AIRFLOW-[0-9]{1,6}", commit_subject): + click.echo(click.style( + 'Note that the commit subject does not reference a ' + 'JIRA issue!', fg='red', bold=True)) + + # -- Note conflicts if had_conflicts: committer_name = run_cmd( "git config --get user.name", echo_cmd=False) @@ -229,14 +220,26 @@ def merge_pr(pr_num, target_ref, title, body, pr_repo_desc, local): 'Committer: %s <%s>' % (committer_name, committer_email)) merge_message_flags.extend(["-m", message]) - # -- add individual commit messages to squash commit + # -- Add PR body to commit message msg = click.style( - 'Would you like to include the individual commit messages ' - 'in the squash commit message?', + 'Would you like to include the PR body in the squash ' + 'commit message?', fg='blue', bold=True) - if pr_commits and click.confirm(msg, default=True, prompt_suffix=''): - for commit in pr_commits: - merge_message_flags.extend(['-m', commit['commit']['message']]) + if body and click.confirm(msg, default=False, prompt_suffix=''): + # We remove @ symbols from the body to avoid triggering e-mails + # to people every time someone creates a public fork of Airflow. + merge_message_flags += ["-m", body.replace("@", "")] + + # -- add individual commit messages to squash commit + if len(pr_commits) > 1: + m = click.style( + 'Would you like to include the individual commit messages ' + 'in the squash commit message?', + fg='blue', bold=True) + if pr_commits and click.confirm(m, default=True, prompt_suffix=''): + for commit in pr_commits: + merge_message_flags.extend( + ['-m', commit['commit']['message']]) # The string "Closes #%s" string is required for GitHub to correctly # close the PR. GitHub will mark the PR as closed, not merged @@ -244,6 +247,26 @@ def merge_pr(pr_num, target_ref, title, body, pr_repo_desc, local): merge_message_flags.extend(["-m", "{} from {}".format( close_msg.capitalize(), pr_repo_desc)]) + # -- set authors and add authors to commit message + commit_authors = run_cmd( + ['git', 'log', 'HEAD..{}'.format(pr_branch_name), + '--pretty=format:%an <%ae>'], echo_cmd=False).split("\n") + distinct_authors = sorted( + set(commit_authors), + key=lambda x: commit_authors.count(x), + reverse=True) + primary_author = click.prompt( + click.style( + 'Enter the primary author in the format of \"name \"', + fg='blue', bold=True), + default=distinct_authors[0]) + if primary_author == "": + primary_author = distinct_authors[0] + + authors = "\n".join(["Author: %s" % a for a in distinct_authors]) + merge_message_flags.append('--author="{}"'.format(primary_author)) + + else: # This will mark the PR as merged merge_message_flags.extend([ From 8f863d7ea7908da0cab417d16f0634d7ec8bf29a Mon Sep 17 00:00:00 2001 From: jlowin Date: Thu, 30 Jun 2016 20:21:09 -0400 Subject: [PATCH 5/7] [AIRFLOW-228] Handle empty version list in PR tool If the filter matched no version names, it would return an empty list and the [0] index would fail. --- dev/airflow-pr | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/dev/airflow-pr b/dev/airflow-pr index e3cb3d3943353..cb807660b44ad 100755 --- a/dev/airflow-pr +++ b/dev/airflow-pr @@ -552,11 +552,14 @@ def resolve_jira_issue(comment=None, jira_id=None, merge_branches=None): fix_versions = None def get_version_json(version_str): - return list(filter(lambda v: v.name == version_str, versions))[0].raw + version_list = list(filter(lambda v: v.name == version_str, versions)) + if version_list: + return version_list[0].raw + else: + return '' if fix_versions and fix_versions != ['']: - jira_fix_versions = list( - map(lambda v: get_version_json(v), fix_versions)) + jira_fix_versions = list(map(get_version_json, fix_versions)) else: jira_fix_versions = None From 7774c3921929f0a86bb79eafb0592cc318a65414 Mon Sep 17 00:00:00 2001 From: jlowin Date: Thu, 30 Jun 2016 20:24:58 -0400 Subject: [PATCH 6/7] [AIRFLOW-260] Handle case when no version is found MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If no version is found, the “.name” attribute can’t be accessed, causing a crash. --- dev/airflow-pr | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/dev/airflow-pr b/dev/airflow-pr index cb807660b44ad..1004a98efa916 100755 --- a/dev/airflow-pr +++ b/dev/airflow-pr @@ -524,7 +524,8 @@ def resolve_jira_issue(comment=None, jira_id=None, merge_branches=None): if versions: default_fix_versions = map( - lambda x: fix_version_from_branch(x, versions).name, merge_branches) + lambda x: fix_version_from_branch(x, versions), merge_branches) + default_fix_versions = [v.name for v in default_fix_versions if v] else: default_fix_versions = [] From 8e0ac53c9cf0426352a020255d6c41522791d4c4 Mon Sep 17 00:00:00 2001 From: jlowin Date: Thu, 30 Jun 2016 20:31:31 -0400 Subject: [PATCH 7/7] [AIRFLOW-260] More graceful exit when issues can't be closed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously, the “fail” function was called, which exited the entire program. By returning from this function, we allow the JIRA loop to resume and users can continue closing other issues. --- dev/airflow-pr | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/dev/airflow-pr b/dev/airflow-pr index 1004a98efa916..9d595b0ef3f8d 100755 --- a/dev/airflow-pr +++ b/dev/airflow-pr @@ -498,13 +498,18 @@ def resolve_jira_issue(comment=None, jira_id=None, merge_branches=None): else: cur_assignee = cur_assignee.displayName + # check if issue was already closed if cur_status == "Resolved" or cur_status == "Closed": + click.echo("JIRA issue {} already has status '{}'".format( + jira_id, cur_status)) + return - fail("JIRA issue %s already has status '%s'" % (jira_id, cur_status)) click.echo(click.style("\n === JIRA %s ===" % jira_id, bold=True)) click.echo("summary:\t%s\nassignee:\t%s\nstatus:\t\t%s\nurl:\t\t%s/%s\n" % ( cur_summary, cur_assignee, cur_status, JIRA_BASE, jira_id)) - continue_maybe('Proceed with AIRFLOW-{}?'.format(jira_id)) + if not click.confirm(click.style( + 'Proceed with AIRFLOW-{}?'.format(jira_id), fg='blue', bold=True)): + return if comment is None: comment = click.prompt(