ZOOKEEPER-2597: Add script to merge PR from Apache git repo to Github #85

eribeiro · 2016-10-08T22:06:12Z

No description provided.

breed

this looks really cool!

i'm not a git master, so i'm a bit confused why we are merging rather than cherry-picking

breed · 2016-10-09T01:39:52Z

zk-merge-pr.py

+
+if __name__ == "__main__":
+    import doctest
+    (failure_count, test_count) = doctest.testmod()


what is this about?

doctest (https://pymotw.com/2/doctest/) is yet another python test framework. This one run tests embedded on comments as these: https://github.com/apache/zookeeper/pull/85/files#diff-9f785a289bdb32d2570e08fbe952fbabR335

breed · 2016-10-09T01:42:26Z

zk-merge-pr.py

+
+    url = pr["url"]
+
+    pr_title = pr["title"]


should we do some basic environ checks here? at least the JIRA_ environment variables should be set to something other than "" right?

we probably want to check that we are running the script in a zookeeper apache repo :)

breed · 2016-10-09T02:00:04Z

zk-merge-pr.py

+            jira_comment = "Issue resolved by pull request %s\n[%s/%s]" % (pr_num, GITHUB_BASE, pr_num)
+            resolve_jira_issues(commit_title, merged_refs, jira_comment)
+        else:
+            print "JIRA_USERNAME and JIRA_PASSWORD not set"


it would be frustrating to get here and have to update the jira by hand. we should make sure we have it username and password and the start and make sure it is valid.

breed · 2016-10-09T02:02:30Z

zk-merge-pr.py

+
+    merged_refs = [target_ref]
+
+    merge_hash = merge_pr(pr_num, target_ref, commit_title, body, pr_repo_desc)


why are we merging? shouldn't we be cherry picking?

IMHO and AFAIK, the merge operation is the default operation (the graph clearly shows a merge of branches instead of a simply cherry picking, even though I would prefer the rebase... but I am no git expert). Cherry picking in this script is being used to "backport" the changes to other branches (possibly older ones). See my comment above.

TL;DR: the script uses the merge by default and cherry pick to backport the commits in the PR to other branches.

breed · 2016-10-09T02:03:09Z

zk-merge-pr.py

+
+    # Merged pull requests don't appear as merged in the GitHub API;
+    # Instead, they're closed by asfgit.
+    merge_commits = \


i don't understand this merge_commits logic. what is going on here? i don't understand why if we have merge_commits we cherry pick it without doing anything else.

It's a bit hazy to me too. As far a I understood, the logic is this:

Say we want to merge PR 34:

line 401 retrieves the json data that describes, among other things, any event associated with PR 34 (pr_events);

Line 432-433 filter only those events that are 'closed' by 'asfgit'. This indicates that PR 34 has already been merged in Apache git repo. Otherwise this list is empty.

If list is not empty (line 435), then at lines 436-447 the script assumes that we want to backport the changes of PR 34 to other (older? newer?) branches. A merge would mess older branches with newer stuff, for example, or create lots of conflicts, so it cherry picks PR 34 commits into those (older) branches. It can be a security fix that need to be backported so it makes sense.

As far as I understand, lines 435-447 are used if we run the script a first time and later realize that PR needs to be applied to other branches as well. Then this logic makes sense if we run it a second/third/... time. I say so, because lines 463-465 basically do the same in the first pass, that is, in the process of merging the PR the first time then the script gives the option of cherry pick the commits of that PR to other branches after merging it into the main branch.

eribeiro

Thanks for stepping up to help with this issue. 😃 I have been sidetracked by other commitments that prevented me from working on it as I would like to. Yet, count on me to help whatever I can.

eribeiro · 2016-10-12T03:21:09Z

zk-merge-pr.py

+
+if __name__ == "__main__":
+    import doctest
+    (failure_count, test_count) = doctest.testmod()


doctest (https://pymotw.com/2/doctest/) is yet another python test framework. This one run tests embedded on comments as these: https://github.com/apache/zookeeper/pull/85/files#diff-9f785a289bdb32d2570e08fbe952fbabR335

eribeiro · 2016-10-12T03:21:34Z

zk-merge-pr.py

+            jira_comment = "Issue resolved by pull request %s\n[%s/%s]" % (pr_num, GITHUB_BASE, pr_num)
+            resolve_jira_issues(commit_title, merged_refs, jira_comment)
+        else:
+            print "JIRA_USERNAME and JIRA_PASSWORD not set"


eribeiro · 2016-10-12T03:21:54Z

zk-merge-pr.py

+
+    url = pr["url"]
+
+    pr_title = pr["title"]


eribeiro · 2016-10-12T03:26:54Z

zk-merge-pr.py

+
+    # Merged pull requests don't appear as merged in the GitHub API;
+    # Instead, they're closed by asfgit.
+    merge_commits = \


It's a bit hazy to me too. As far a I understood, the logic is this:

Say we want to merge PR 34:

line 401 retrieves the json data that describes, among other things, any event associated with PR 34 (pr_events);

Line 432-433 filter only those events that are 'closed' by 'asfgit'. This indicates that PR 34 has already been merged in Apache git repo. Otherwise this list is empty.

If list is not empty (line 435), then at lines 436-447 the script assumes that we want to backport the changes of PR 34 to other (older? newer?) branches. A merge would mess older branches with newer stuff, for example, or create lots of conflicts, so it cherry picks PR 34 commits into those (older) branches. It can be a security fix that need to be backported so it makes sense.

As far as I understand, lines 435-447 are used if we run the script a first time and later realize that PR needs to be applied to other branches as well. Then this logic makes sense if we run it a second/third/... time. I say so, because lines 463-465 basically do the same in the first pass, that is, in the process of merging the PR the first time then the script gives the option of cherry pick the commits of that PR to other branches after merging it into the main branch.

eribeiro · 2016-10-12T03:42:32Z

zk-merge-pr.py

+
+    merged_refs = [target_ref]
+
+    merge_hash = merge_pr(pr_num, target_ref, commit_title, body, pr_repo_desc)


IMHO and AFAIK, the merge operation is the default operation (the graph clearly shows a merge of branches instead of a simply cherry picking, even though I would prefer the rebase... but I am no git expert). Cherry picking in this script is being used to "backport" the changes to other branches (possibly older ones). See my comment above.

TL;DR: the script uses the merge by default and cherry pick to backport the commits in the PR to other branches.

eribeiro · 2016-10-12T04:31:55Z

@breed A quick check shows that this script is Python2. It will not run on Python3, afaik. It would require many small changes to make it compatible to Python3.

breed · 2016-10-17T22:51:28Z

test of jira bridging

breed · 2016-10-25T04:59:28Z

i think there are still quite a few improvements that can be made to this script, for example it assumes that the repo 'apache-github' is setup, so it would be nice to check for that at the start of the script and then print out how to set it up if it isn't setup.

i'm thinking that we should get this checked in and then iterate on it as we use it. what do others think?

eribeiro · 2016-10-25T11:48:40Z

@breed I was thinking of adding a confluence step-by-step guide on how to set up the environment variables too.

eribeiro · 2016-10-25T12:53:51Z

@breed I have just added two checks to the main() function. One does check if JIRA credentials are set and other one tries to infer if the remote repos are compliant, that is, they point to a 'zookeeper' repo and that PR_REMOTE_NAME and PUSH_REMOTE_NAME are present. I don't like the echo'ing of the git commands, but couldn't see where to disable it at first look.

Check git remote name and URL Add .strip() to trim input strings Add '.git' suffix to check_git_remote

eribeiro · 2016-10-26T02:55:24Z

@breed I guess this PR is a good candidate to backport to other branches (4.5, etc) besides master, don't you think? I mean, if the GH PR targets another branch then the script can disappear when that branch is checked out, right? Does it make sense what I am thinking?

breed · 2016-10-26T03:27:21Z

i don't think we need to backport it. you don't have to have the branch checked out to commit to it. (if i understand things correctly.)

edwardoliveira · 2016-10-26T16:28:49Z

@breed I had understood the other way around 🤔 , that is, it creates a local branch from the target branch in addition to a local branch to the PR. Then it checkout the target branch and merge the PR branch. Need to test this tough.

Oh, I have a suggestion to add a else clause to check_jira_env to account for the lack of jyra-python lib at the start of the script. If you like then please add it to the current file. 😸

check_jira_env():
        if JIRA_IMPORTED:
            if JIRA_USERNAME.strip() == "" or JIRA_PASSWORD.strip() == "":
                msg ="JIRA credentials are not set. Want to continue?"
                continue_maybe(msg)
        # ADD THIS SNIPPET BELOW
       else:
           msg = "Could not find jira-python library!\n" + \
                      "Quit now and run `sudo pip install jira` to install it.\n" + \
                      "Continue?"                                                                                                                            
           continue_maybe(msg)

hanm · 2016-10-26T17:50:56Z

+1 great job Eddie.

edwardoliveira · 2016-10-26T18:33:54Z

Thanks @hanm ! 😃

Author: Edward Ribeiro <edward.ribeiro@gmail.com> Reviewers: Benjamin Reed <breed@apache.org> Closes apache#85 from eribeiro/ZOOKEEPER-2597

This change allows ZooKeeper server to read super user id string from system property, and parse it using delimiter "," to multiple client ids. Originally only one client id can be super user, this commit opens up the super user privilege to multiple clients. This commit includes non-backward-compatible changes in X509AuthenticationConfig class: setZnodeGroupAclSuperUserId is renamed to setZnodeGroupAclSuperUserIdStr getZnodeGroupAclSuperUserId is renamed to getZnodeGroupAclSuperUserIds, and return type is changed from String to Set Test modified: X509ZNodeGroupAclProviderTest.testSuperUser Test passed: all tests under org.apache.zookeeper.server.auth.znode.groupacl

Author: Edward Ribeiro <edward.ribeiro@gmail.com> Reviewers: Benjamin Reed <breed@apache.org> Closes apache#85 from eribeiro/ZOOKEEPER-2597

ZOOKEEPER-2597: Add script to merge PR from Apache git repo to Github

d62c510

breed reviewed Oct 9, 2016

View reviewed changes

eribeiro commented Oct 12, 2016

View reviewed changes

Check JIRA env variables

dbc15d1

Check git remote name and URL Add .strip() to trim input strings Add '.git' suffix to check_git_remote

asfgit closed this in f6349d1 Oct 26, 2016

ollie314 mentioned this pull request Oct 26, 2016

ZOOKEEPER-2597: Add script to merge PR from Apache git repo to Github ollie314/zookeeper#4

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ZOOKEEPER-2597: Add script to merge PR from Apache git repo to Github #85

ZOOKEEPER-2597: Add script to merge PR from Apache git repo to Github #85

eribeiro commented Oct 8, 2016

breed left a comment

breed Oct 9, 2016

eribeiro Oct 12, 2016

breed Oct 9, 2016

eribeiro Oct 12, 2016

breed Oct 9, 2016

eribeiro Oct 12, 2016

breed Oct 9, 2016

eribeiro Oct 12, 2016 •

edited

breed Oct 9, 2016

eribeiro Oct 12, 2016 •

edited

eribeiro left a comment

eribeiro Oct 12, 2016

eribeiro Oct 12, 2016

eribeiro Oct 12, 2016

eribeiro Oct 12, 2016 •

edited

eribeiro Oct 12, 2016 •

edited

eribeiro commented Oct 12, 2016

breed commented Oct 17, 2016

breed commented Oct 25, 2016

eribeiro commented Oct 25, 2016

eribeiro commented Oct 25, 2016

eribeiro commented Oct 26, 2016

breed commented Oct 26, 2016

edwardoliveira commented Oct 26, 2016

hanm commented Oct 26, 2016

edwardoliveira commented Oct 26, 2016


		merged_refs = [target_ref]

		merge_hash = merge_pr(pr_num, target_ref, commit_title, body, pr_repo_desc)

ZOOKEEPER-2597: Add script to merge PR from Apache git repo to Github #85

ZOOKEEPER-2597: Add script to merge PR from Apache git repo to Github #85

Conversation

eribeiro commented Oct 8, 2016

breed 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

eribeiro Oct 12, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eribeiro Oct 12, 2016 • edited

Choose a reason for hiding this comment

eribeiro 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

eribeiro Oct 12, 2016 • edited

Choose a reason for hiding this comment

eribeiro Oct 12, 2016 • edited

Choose a reason for hiding this comment

eribeiro commented Oct 12, 2016

breed commented Oct 17, 2016

breed commented Oct 25, 2016

eribeiro commented Oct 25, 2016

eribeiro commented Oct 25, 2016

eribeiro commented Oct 26, 2016

breed commented Oct 26, 2016

edwardoliveira commented Oct 26, 2016

hanm commented Oct 26, 2016

edwardoliveira commented Oct 26, 2016

eribeiro Oct 12, 2016 •

edited

eribeiro Oct 12, 2016 •

edited

eribeiro Oct 12, 2016 •

edited

eribeiro Oct 12, 2016 •

edited