add support for --merge-pr by boegel · Pull Request #2266 · easybuilders/easybuild-framework

boegel · 2017-07-07T23:08:25Z

support for eb --merge-pr, meant to help EasyBuild maintainers merging PRs (easyconfigs only for now)

checks the requirements documented at http://easybuild.readthedocs.io/en/latest/Contributing.html#requirements-for-pull-requests as best as possible, see implementation of the check_pr_eligible_to_merge function

verdurin

Minor comments

verdurin · 2017-07-08T05:27:37Z

+    :param silent: be silent (only log, don't print)
+    :param prefix: include message prefix characters ('== ')
+    :param newline: end message with newline
+    :param stderr: print to stderr rather than stderr


Is this a typo?

Should be 'print to stdout rather than stderr'?

The other way around print to stderr rather than stdout

verdurin · 2017-07-08T05:29:30Z

        status, data = url.get(**kwargs)
    except socket.gaierror, err:
-        _log.warning("Error occured while performing get request: %s" % err)
+        _log.warning("Error occured while performing get request: %s", err)


verdurin · 2017-07-08T05:29:56Z

+    try:
+        status, data = url.put(**kwargs)
+    except socket.gaierror, err:
+        _log.warning("Error occured while performing put request: %s", err)


verdurin · 2017-07-08T05:32:33Z

+    found_lgtm = False
+    for comment in pr_data['issue_comments']['bodies']:
+        if 'lgtm' in comment:
+            found_lgtm = True


According to this all maintainers have to use the formulation 'lgtm' then, for this test to work? Worth mentioning that in the contributors' guide, if it isn't there already.

I want to relax this a bit and also make this check pass when an approved review is submitted, especially since that's required now to merge (via the web interface).

I agree on checking against approved review. Cleaner and less prone to personal quirks.

…merge own PRs

boegel · 2017-07-08T07:51:26Z

@verdurin Please re-review, and maybe also give it a try yourself?

Example usage (I'll write up documentation too) in dry run mode (only difference is that submitted the comment and actually merging are not done if PR is eligible for merging):

$ eb --merge-pr 4827 -x
== temporary log file in case of crash /tmp/eb-Gz20kc/easybuild-G3wn6E.log

easybuilders/easybuild-easyconfigs PR #4827 was submitted by nathanhaigh, you are using GitHub account 'boegel'

Checking eligibility of easybuilders/easybuild-easyconfigs PR #4827 for merging...
* targets develop branch: OK
* test suite passes: OK
* last test report is successful: (no test reports found) => not eligible for merging!
* approved review: MISSING => not eligible for merging!
* milestone is set: no milestone found => not eligible for merging!

WARNING: Review indicates this PR should not be merged (use -f/--force to do so anyway)

boegel · 2017-07-08T08:16:24Z

documentation update @ easybuilders/easybuild#356

damianam · 2017-07-10T14:42:46Z

+def check_pr_eligible_to_merge(pr_data):
+    """
+    Check whether PR is eligible for merging.
+


More of a question rather than a comment, this empty line sometimes exists sometimes doesn't. Shouldn't we stick to one style? Or is there a reason for it? I favour having it.

We should stick to one style, and I generally prefer having a blank line there as well, but the only way to make this consistent is to have an automated style check for it...

damianam · 2017-07-10T14:54:33Z

+                print_warning("Failed to determine outcome of test report for comment:\n%s" % comment)
+
+        if 'lgtm' in comment:
+            found_lgtm = True


Isn't this useless?

This didn't get cleaned up, indeed, thanks.

damianam · 2017-07-10T15:35:21Z

+    # also fetch status of last commit
+    pr_head_sha = pr_data['head']['sha']
+    status_url = lambda g: g.repos[pr_target_account][pr_target_repo].commits[pr_head_sha].status
+    status, status_data = github_api_get_request(status_url, github_user)


If status is not used, maybe it makes sense to replace it with _ to make it clear? Even though I'd suggest to check the status here too, as individual consecutive requests might return different status.

fixed by checking status, thanks for bringing this up

damianam · 2017-07-10T15:35:56Z

+
+    # also fetch comments
+    comments_url = lambda g: g.repos[pr_target_account][pr_target_repo].issues[pr].comments
+    status, comments_data = github_api_get_request(comments_url, github_user)


damianam · 2017-07-10T15:36:10Z

+
+    # also fetch reviews
+    reviews_url = lambda g: g.repos[pr_target_account][pr_target_repo].pulls[pr].reviews
+    status, reviews_data = github_api_get_request(reviews_url, github_user)


damianam · 2017-07-10T15:38:26Z

+    status, reviews_data = github_api_get_request(reviews_url, github_user)
+    pr_data['reviews'] = reviews_data
+
+    force = build_option('force')


I wouldn't give this possibility. If it is not elegible for merging it needs more careful evaluation. Slapping --force is an easy way out for potentially lazy maintainers.

I disagree here, in rare cases it makes sense to merge a PR that doesn't strictly meet all requirements.

For example, when only trivial changes are made that do not add new easyconfigs and do not alter the semantics of existing easyconfigs, we can merge it without requiring a test report. One recent example is easybuilders/easybuild-easyconfigs#4815...

I would rather have maintainers use --merge-pr X --force after running it without --force first, than favouring manual merges where it's easier to overlook one of the other requirements. Of course, people shouldn't be using --force all the time...

That's a good point and a good example. I was thinking more about new additions or version changes, than that kind of small tuning. I guess it makes sense then. But I wonder if a middle ground is possible. In other words, check what is the nature of the changes, and reject to merge, even with --force, if new files are added, or there are changes in version, toolchain, configopts, etc, but allow it if the changes are just comments, or description, whatis and maybe one or two more parameters. That's probably a PITA to implement and possibly not worth the effort though, as long as all behave responsibly ;-)

That would be nice, but that's definitely tricky to implement, and I'm not sure it'll be worth the trouble...
We can always look into that enhancement later though, if we find it's really necessary, but I don't expect that maintainers will go and 'abuse' --merge-pr X --force, to avoid tarring & feathering by the other maintainers. ;-)

…1 release notes

damianam · 2017-07-10T19:38:50Z

I haven't have time to take a look at the tests, but otherwise lgtm. I'll try to take a look at the tests tomorrow. Anyway, maybe @vanzod or @migueldiascosta can give their feedback too before merging?

migueldiascosta · 2017-07-11T01:52:31Z

+
+    pr_target_account = build_option('pr_target_account')
+    pr_target_repo = build_option('pr_target_repo')
+


make sure pr_target_repo == GITHUB_EASYCONFIGS_REPO?

Well, we could be a bit smarter, and skip the check for a successful test report for other repositories, since the other checks are just as important for framework & easyblocks?

I'd even argue that for easyblocks a successful test report might be just as important as for the easyconfigs repo, given that both decide how the software is built. But we would have to make sure that the easyblock is used in the easyconfigs being built. In any case, I thought that at this moment the github integration worked just with the easyconfigs repo, so I wouldn't delay this PR because of this.

We don't have a way to submit test reports for easyblock PRs yet.

It's true that currently most GitHub-related options only work with the easyconfigs repo for now, but the intention is to extend that to also easyblocks/framework, certainly for --new-pr and --update-pr.

The extra effort here to make --merge-pr also work well for the easyblocks & framework repos is minimal, see 17789d5.

migueldiascosta

lgtm

verdurin · 2017-07-11T09:30:54Z

Looks good, just wanted to reiterate my earlier comment that if we're expecting 'lgtm' and 'Going in, thanks...' for each PR, then that should be documented.

damianam · 2017-07-11T09:42:36Z

Seems like it lgte (looks good to everyone ;-)). Going in!

boegel · 2017-07-11T10:38:03Z

@verdurin the lgtm is no longer needed, --merge-pr will only look for an approved review (which basically is a lgtm statement)

the Going in, thanks is just being friendly, but it's not a strict requirement ;)

boegel added 6 commits July 7, 2017 23:09

add --merge-pr & (empty) corresponding merge_pr function

36431db

enhance print_msg to support printing messages to stderr

6264746

implement check_pr_eligible_to_merge function

6695131

complete implementation of merge_pr function

ec93b26

add test for --merge-pr

c3a5d2d

fix broken test for --list-software

d203a35

boegel added this to the 3.4.0 milestone Jul 7, 2017

boegel requested review from a team, migueldiascosta, vanzod and verdurin July 7, 2017 23:08

verdurin requested changes Jul 8, 2017

View reviewed changes

boegel added 3 commits July 8, 2017 08:57

fix typo in docstring for print_msg

3bb0062

occured -> occurred

8950f37

check for approvied review rather than 'lgtm' in comments, refuse to …

2e411c8

…merge own PRs

boegel mentioned this pull request Jul 8, 2017

add documentation on --merge-pr easybuilders/easybuild#356

Merged

boegel changed the base branch from develop to eb331 July 9, 2017 20:51

boegel modified the milestones: 3.3.1, 3.4.0 Jul 9, 2017

damianam requested changes Jul 10, 2017

View reviewed changes

fix @damianam's remarks

03a2d3f

boegel added a commit to boegel/easybuild-framework that referenced this pull request Jul 10, 2017

also include easybuilders#2262 & easybuilders#2266 in EasyBuild v3.3.…

31b2bb1

…1 release notes

damianam approved these changes Jul 10, 2017

View reviewed changes

vanzod approved these changes Jul 10, 2017

View reviewed changes

migueldiascosta reviewed Jul 11, 2017

View reviewed changes

boegel added 2 commits July 11, 2017 09:43

only require test reports for easyconfig PRs

17789d5

enhance test for --merge-pr to also test on an easyblocks PR

30ad5ec

migueldiascosta approved these changes Jul 11, 2017

View reviewed changes

easybuilders deleted a comment from boegelbot Jul 11, 2017

damianam merged commit f75d294 into easybuilders:eb331 Jul 11, 2017

boegel deleted the merge_pr branch July 11, 2017 10:38


		pr_target_account = build_option('pr_target_account')
		pr_target_repo = build_option('pr_target_repo')

Conversation

boegel commented Jul 7, 2017

Uh oh!

verdurin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

boegel commented Jul 8, 2017

Uh oh!

boegel commented Jul 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

damianam commented Jul 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

migueldiascosta left a comment

Choose a reason for hiding this comment

Uh oh!

verdurin commented Jul 11, 2017

Uh oh!

damianam commented Jul 11, 2017

Uh oh!

boegel commented Jul 11, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants