Skip to content
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
8fdbdc1
initial support for --close-pr
migueldiascosta Feb 12, 2018
964f696
fix message and long line in close_pr
migueldiascosta Feb 12, 2018
fd5fcbb
improve comment to post in close_pr
migueldiascosta Feb 13, 2018
07467c6
ammend close_pr message
migueldiascosta Feb 14, 2018
6b08c43
flesh out common code block for fetching pr data
migueldiascosta Mar 20, 2018
6d51226
fix style
migueldiascosta Mar 21, 2018
50c8729
add test for fetch_pr_data function
migueldiascosta Mar 21, 2018
979d888
add predefined close-pr reasons and messages
migueldiascosta Apr 4, 2018
5ffc967
add supported values to --close-pr-reasons help message
migueldiascosta Apr 5, 2018
e5898ba
automatically check for 'inactive' reason if no reason specified with…
migueldiascosta Apr 5, 2018
1efaf34
fix style
migueldiascosta Apr 5, 2018
c0345f1
automatically check for 'archived' reason if no reason specified with…
migueldiascosta Apr 6, 2018
29acced
automatically check for 'obsolete' reason if no reason specified with…
migueldiascosta Apr 10, 2018
7bf3a2a
move fetching additional fields into fetch_pr_data
migueldiascosta Apr 10, 2018
1bf0dc4
more verbose output when looking for reasons to close pr
migueldiascosta Apr 10, 2018
91f46e2
sync with develop
migueldiascosta Nov 20, 2018
34f6996
sync with develop
migueldiascosta Nov 22, 2018
03583e7
sync with develop
migueldiascosta Nov 23, 2018
c433ecc
minor code cleanup, extend test for fetch_pr_data
boegel Dec 10, 2018
5fd515c
more code cleanup in tools/github.py
boegel Dec 10, 2018
fb89e0b
Merge pull request #6 from boegel/close_pr
migueldiascosta Dec 11, 2018
580feb2
Merge branch 'develop' into close_pr
migueldiascosta Dec 11, 2018
527b41e
add test for close_pr
boegel Dec 12, 2018
eda7b5c
add test for reasons_for_closing
boegel Dec 12, 2018
6e2cc2d
Merge pull request #7 from boegel/close_pr
migueldiascosta Dec 13, 2018
eb4eb0b
Merge branch 'close_pr' of github.com:migueldiascosta/easybuild-frame…
migueldiascosta Dec 13, 2018
fc2e7bf
skip tests if not github token is available
migueldiascosta Dec 13, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion easybuild/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@
from easybuild.tools.docs import list_software
from easybuild.tools.filetools import adjust_permissions, cleanup, write_file
from easybuild.tools.github import check_github, find_easybuild_easyconfig, install_github_token
from easybuild.tools.github import new_pr, merge_pr, update_pr
from easybuild.tools.github import close_pr, new_pr, merge_pr, update_pr
from easybuild.tools.hooks import START, END, load_hooks, run_hook
from easybuild.tools.modules import modules_tool
from easybuild.tools.options import parse_external_modules_metadata, process_software_build_specs, use_color
Expand Down Expand Up @@ -284,6 +284,9 @@ def main(args=None, logfile=None, do_build=None, testing=False, modtool=None):
elif options.install_github_token:
install_github_token(options.github_user, silent=build_option('silent'))

elif options.close_pr:
close_pr(options.close_pr, reasons=options.close_pr_msg)

elif options.merge_pr:
merge_pr(options.merge_pr)

Expand All @@ -303,6 +306,7 @@ def main(args=None, logfile=None, do_build=None, testing=False, modtool=None):
options.install_github_token,
options.list_installed_software,
options.list_software,
options.close_pr,
options.merge_pr,
options.review_pr,
options.terse,
Expand Down
86 changes: 71 additions & 15 deletions easybuild/tools/github.py
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
import tempfile
import time
import urllib2
from datetime import datetime, timedelta
from distutils.version import LooseVersion
from vsc.utils import fancylogger
from vsc.utils.missing import nub
Expand Down Expand Up @@ -96,6 +97,9 @@
KEYRING_GITHUB_TOKEN = 'github_token'
URL_SEPARATOR = '/'

VALID_CLOSE_PR_REASONS = {'archived': 'uses an archived toolchain',
'inactive': 'no activity for > 6 months',
'obsolete': 'obsoleted by more recent PRs'}

class Githubfs(object):
"""This class implements some higher level functionality on top of the Github api"""
Expand Down Expand Up @@ -369,12 +373,8 @@ def fetch_easyconfigs_from_pr(pr, path=None, github_user=None):
mkdir(path, parents=True)

_log.debug("Fetching easyconfigs from PR #%s into %s" % (pr, path))
pr_url = lambda g: g.repos[GITHUB_EB_MAIN][GITHUB_EASYCONFIGS_REPO].pulls[pr]

status, pr_data = github_api_get_request(pr_url, github_user)
if status != HTTP_STATUS_OK:
raise EasyBuildError("Failed to get data for PR #%d from %s/%s (status: %d %s)",
pr, GITHUB_EB_MAIN, GITHUB_EASYCONFIGS_REPO, status, pr_data)
status, pr_data, pr_url = fetch_pr_data(pr, GITHUB_EB_MAIN, GITHUB_EASYCONFIGS_REPO, github_user)

# if PR is open and mergable, download develop and patch
stable = pr_data['mergeable_state'] == GITHUB_MERGEABLE_STATE_CLEAN
Expand Down Expand Up @@ -970,6 +970,60 @@ def not_eligible(msg):
return res


def close_pr(pr, reasons):
"""
Close specified pull request
"""
github_user = build_option('github_user')
if github_user is None:
raise EasyBuildError("GitHub user must be specified to use --close-pr")

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

status, pr_data, pr_url = fetch_pr_data(pr, pr_target_account, pr_target_repo, github_user)

if pr_data['state'] == GITHUB_STATE_CLOSED:
raise EasyBuildError("PR #%d from %s/%s is already closed.", pr, pr_target_account, pr_target_repo)

pr_owner = pr_data['user']['login']
msg = "\n%s/%s PR #%s was submitted by %s, " % (pr_target_account, pr_target_repo, pr, pr_owner)
msg += "you are using GitHub account '%s'\n" % github_user
print_msg(msg, prefix=False)

dry_run = build_option('dry_run') or build_option('extended_dry_run')

if not reasons:
relevant_reasons = []
if datetime.now() - datetime.strptime(pr_data['updated_at'], "%Y-%m-%dT%H:%M:%SZ") > timedelta(days=180):
relevant_reasons.append('inactive')
# TODO: check the other valid reasons
if not relevant_reasons:
raise EasyBuildError("No reason specified and none found from PR data, "
"please use --close-pr-reasons or --close-pr-msg")
else:
reasons = ", ".join([VALID_CLOSE_PR_REASONS[reason] for reason in relevant_reasons])

msg = "@%s, this PR is being closed for the following reason(s): %s.\n" % (pr_data['user']['login'], reasons)
msg += "Please don't hesitate to reopen this PR or add a comment if you feel this contribution is still relevant.\n"
msg += "For more information on our policy w.r.t. closing PRs, see "
msg += "https://easybuild.readthedocs.io/en/latest/Contributing.html#why-a-pull-request-may-be-closed-by-a-maintainer"
post_comment_in_issue(pr, msg, account=pr_target_account, repo=pr_target_repo, github_user=github_user)

if dry_run:
print_msg("[DRY RUN] Closed %s/%s pull request #%s" % (pr_target_account, pr_target_repo, pr), prefix=False)
else:
github_token = fetch_github_token(github_user)
if github_token is None:
raise EasyBuildError("GitHub token for user '%s' must be available to use --close-pr", github_user)
g = RestClient(GITHUB_API_URL, username=github_user, token=github_token)
pull_url = g.repos[pr_target_account][pr_target_repo].pulls[pr]
body = {'state': 'closed'}
status, data = pull_url.post(body=body)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a lot of duplication here too, maybe time for a pr_action function that gets PR# + body as arguments (and github_token as optional argument)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@migueldiascosta Does this suggestion still make sense, or not?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@boegel I don't know - each case is slightly different, but I may be missing an obvious generalization

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just took a look myself, it's indeed not entirely trivial, so let's leave it as is (for now).

if not status == HTTP_STATUS_OK:
raise EasyBuildError("Failed to close PR #%s; status %s, data: %s", pr, status, data)


def merge_pr(pr):
"""
Merge specified pull request
Expand All @@ -981,11 +1035,7 @@ def merge_pr(pr):
pr_target_account = build_option('pr_target_account')
pr_target_repo = build_option('pr_target_repo')

pr_url = lambda g: g.repos[pr_target_account][pr_target_repo].pulls[pr]
status, pr_data = github_api_get_request(pr_url, github_user)
if status != HTTP_STATUS_OK:
raise EasyBuildError("Failed to get data for PR #%d from %s/%s (status: %d %s)",
pr, pr_target_account, pr_target_repo, status, pr_data)
status, pr_data, pr_url = fetch_pr_data(pr, pr_target_account, pr_target_repo, github_user)

msg = "\n%s/%s PR #%s was submitted by %s, " % (pr_target_account, pr_target_repo, pr, pr_data['user']['login'])
msg += "you are using GitHub account '%s'\n" % github_user
Expand Down Expand Up @@ -1175,11 +1225,7 @@ def update_pr(pr, paths, ecs, commit_msg=None):
pr_target_account = build_option('pr_target_account')
pr_target_repo = build_option('pr_target_repo')

pr_url = lambda g: g.repos[pr_target_account][pr_target_repo].pulls[pr]
status, pr_data = github_api_get_request(pr_url, github_user)
if status != HTTP_STATUS_OK:
raise EasyBuildError("Failed to get data for PR #%d from %s/%s (status: %d %s)",
pr, pr_target_account, pr_target_repo, status, pr_data)
status, pr_data, pr_url = fetch_pr_data(pr, pr_target_account, pr_target_repo, github_user)

# branch that corresponds with PR is supplied in form <account>:<branch_label>
account = pr_data['head']['label'].split(':')[0]
Expand Down Expand Up @@ -1517,3 +1563,13 @@ def find_easybuild_easyconfig(github_user=None):

eb_file = os.path.join(eb_parent_path, fn)
return eb_file


def fetch_pr_data(pr, pr_target_account, pr_target_repo, github_user):
"""Fetch PR data from GitHub"""
pr_url = lambda g: g.repos[pr_target_account][pr_target_repo].pulls[pr]
status, pr_data = github_api_get_request(pr_url, github_user)
if status != HTTP_STATUS_OK:
raise EasyBuildError("Failed to get data for PR #%d from %s/%s (status: %d %s)",
pr, pr_target_account, pr_target_repo, status, pr_data)
return status, pr_data, pr_url
22 changes: 21 additions & 1 deletion easybuild/tools/options.py
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@
from easybuild.tools.environment import restore_env, unset_env_vars
from easybuild.tools.filetools import CHECKSUM_TYPE_SHA256, CHECKSUM_TYPES, mkdir
from easybuild.tools.github import GITHUB_EB_MAIN, GITHUB_EASYCONFIGS_REPO, HAVE_GITHUB_API, HAVE_KEYRING
from easybuild.tools.github import VALID_CLOSE_PR_REASONS
from easybuild.tools.github import fetch_github_token
from easybuild.tools.hooks import KNOWN_HOOKS
from easybuild.tools.include import include_easyblocks, include_module_naming_schemes, include_toolchains
Expand Down Expand Up @@ -126,7 +127,6 @@ def eb_shell_quote(token):
DEFAULT_SYS_CFGFILES = [f for d in XDG_CONFIG_DIRS for f in sorted(glob.glob(os.path.join(d, 'easybuild.d', '*.cfg')))]
DEFAULT_USER_CFGFILE = os.path.join(XDG_CONFIG_HOME, 'easybuild', 'config.cfg')


_log = fancylogger.getLogger('options', fname=False)


Expand Down Expand Up @@ -567,6 +567,10 @@ def github_options(self):
'github-user': ("GitHub username", str, 'store', None),
'github-org': ("GitHub organization", str, 'store', None),
'install-github-token': ("Install GitHub token (requires --github-user)", None, 'store_true', False),
'close-pr': ("Close pull request", int, 'store', None, {'metavar': 'PR#'}),
'close-pr-msg': ("Custom close message for pull request closed with --close-pr; ", str, 'store', None),
'close-pr-reasons': ("Close reason for pull request closed with --close-pr; "
"supported values: %s" % ", ".join(VALID_CLOSE_PR_REASONS), str, 'store', None),
'merge-pr': ("Merge pull request", int, 'store', None, {'metavar': 'PR#'}),
'new-pr': ("Open a new pull request", None, 'store_true', False),
'pr-branch-name': ("Branch name to use for new PRs; '<timestamp>_new_pr_<name><version>' if unspecified",
Expand Down Expand Up @@ -786,6 +790,10 @@ def postprocess(self):
if self.options.optarch and not self.options.job:
self._postprocess_optarch()

# make sure --close-pr-reasons has a valid format and if so use it to set close-pr-msg
if self.options.close_pr_reasons:
self._postprocess_close_pr_reasons()

# handle configuration options that affect other configuration options
self._postprocess_config()

Expand Down Expand Up @@ -819,6 +827,18 @@ def _postprocess_optarch(self):
else:
self.log.info("Keeping optarch raw: %s", self.options.optarch)

def _postprocess_close_pr_reasons(self):
"""Postprocess --close-pr-reasons options"""
if self.options.close_pr_msg:
raise EasyBuildError("Please either specify predefined reasons with --close-pr-reasons or " +
"a custom message with--close-pr-msg")

reasons = self.options.close_pr_reasons.split(',')
if any([reason not in VALID_CLOSE_PR_REASONS.keys() for reason in reasons]):
raise EasyBuildError("Argument to --close-pr_reasons must be a comma separated list of valid reasons " +
"among %s" % VALID_CLOSE_PR_REASONS.keys())
self.options.close_pr_msg = ", ".join([VALID_CLOSE_PR_REASONS[reason] for reason in reasons])

def _postprocess_include(self):
"""Postprocess --include options."""
# set up included easyblocks, module naming schemes and toolchains/toolchain components
Expand Down
12 changes: 12 additions & 0 deletions test/framework/github.py
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,18 @@ def test_read(self):
except (IOError, OSError):
pass

def test_fetch_pr_data(self):
"""Test fetch_pr_data function."""
if self.github_token is None:
print "Skipping test_fetch_pr_data, no GitHub token available?"
return

status, pr_data, pr_url = gh.fetch_pr_data(1, GITHUB_USER, GITHUB_REPO, GITHUB_TEST_ACCOUNT)

self.assertEquals(gh.HTTP_STATUS_OK, status)
self.assertEquals(pr_data['number'], 1)
self.assertEquals(pr_data['title'], "a pr")

def test_fetch_easyconfigs_from_pr(self):
"""Test fetch_easyconfigs_from_pr function."""
if self.github_token is None:
Expand Down