Skip to content
Merged
25 changes: 25 additions & 0 deletions easybuild/framework/easyconfig/tools.py
Original file line number Diff line number Diff line change
Expand Up @@ -499,6 +499,31 @@ def review_pr(pr, colored=True, branch='develop'):

return '\n'.join(lines)

def preview_pr(paths, colored=True, branch='develop'):
"""
Print multi-diff overview between easyconfigs in new PR and specified branch.
:param paths: path tuples (path, generated)
:param colored: boolean indicating whether a colored multi-diff should be generated
:param branch: easybuild-easyconfigs branch to compare with
"""
tmpdir = tempfile.mkdtemp()

download_repo_path = download_repo(branch=branch, path=tmpdir)
repo_path = os.path.join(download_repo_path, 'easybuild', 'easyconfigs')
pr_files = [path[0] for path in paths]

lines = []
ecs, _ = parse_easyconfigs([(fp, False) for fp in pr_files], validate=False)
for ec in ecs:
files = find_related_easyconfigs(repo_path, ec['ec'])
_log.debug("File in new PR %s has these related easyconfigs: %s" % (ec['spec'], files))
if files:
lines.append(multidiff(ec['spec'], files, colored=colored))
else:
lines.extend(['', "(no related easyconfigs found for %s)\n" % os.path.basename(ec['spec'])])

return '\n'.join(lines)
Copy link
Member

Choose a reason for hiding this comment

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

@migueldiascosta This is working as expected now, but there's way too much copy-pasting going on here when comparing preview_pr with review_pr.
The only thing that differs is the list of files (pr_files) and a part of the log message.

Can we create a private method (_pr_diff_common or something like that) which accepts these two as additional arguments?

Copy link
Member Author

@migueldiascosta migueldiascosta Oct 24, 2017

Choose a reason for hiding this comment

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

@boegel definitely, will do

Copy link
Member

Choose a reason for hiding this comment

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

Current approach of enhancing the existing review_pr is way better 👍



def dump_env_script(easyconfigs):
"""
Expand Down
10 changes: 7 additions & 3 deletions easybuild/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@
from easybuild.framework.easyconfig.style import cmdline_easyconfigs_style_check
from easybuild.framework.easyconfig.tools import alt_easyconfig_paths, categorize_files_by_type, dep_graph
from easybuild.framework.easyconfig.tools import det_easyconfig_paths, dump_env_script, get_paths_for
from easybuild.framework.easyconfig.tools import parse_easyconfigs, review_pr, skip_available
from easybuild.framework.easyconfig.tools import parse_easyconfigs, preview_pr, review_pr, skip_available
from easybuild.framework.easyconfig.tweak import obtain_ec_for, tweak
from easybuild.tools.config import find_last_log, get_repository, get_repositorypath, build_option
from easybuild.tools.docs import list_software
Expand Down Expand Up @@ -375,7 +375,7 @@ def main(args=None, logfile=None, do_build=None, testing=False, modtool=None):

forced = options.force or options.rebuild
dry_run_mode = options.dry_run or options.dry_run_short
new_update_pr = options.new_pr or options.update_pr
new_update_pr = options.new_pr or options.update_pr or options.preview_pr
Copy link
Member

Choose a reason for hiding this comment

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

maybe this should be renamed to new_update_preview_pr?


# skip modules that are already installed unless forced, or unless an option is used that warrants not skipping
if not (forced or dry_run_mode or options.extended_dry_run or new_update_pr or options.inject_checksums):
Expand All @@ -402,7 +402,11 @@ def main(args=None, logfile=None, do_build=None, testing=False, modtool=None):

# creating/updating PRs
if new_update_pr:
if options.new_pr:
if options.preview_pr:
print preview_pr(paths, colored=use_color(options.color))
cleanup(logfile, eb_tmpdir, testing, silent=True)
sys.exit(0)
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, can we avoid doing cleanup/exit here? How is this handled for --new-pr?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's because options.new_pr is in no_ec_opts, there's a cleanup later for those, I suppose options.preview_pr can also be added to them (although it's a bit counterintuitive that these don't require any easyconfigs to be specified?)

elif options.new_pr:
Copy link
Member

Choose a reason for hiding this comment

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

keep these alphabetically sorted, so new_pr before preview_pr

new_pr(categorized_paths, ordered_ecs, title=options.pr_title, descr=options.pr_descr,
commit_msg=options.pr_commit_msg)
else:
Expand Down
1 change: 1 addition & 0 deletions easybuild/tools/options.py
Original file line number Diff line number Diff line change
Expand Up @@ -569,6 +569,7 @@ def github_options(self):
'pr-target-branch': ("Target branch for new PRs", str, 'store', 'develop'),
'pr-target-repo': ("Target repository for new/updating PRs", str, 'store', GITHUB_EASYCONFIGS_REPO),
'pr-title': ("Title for new pull request created with --new-pr", str, 'store', None),
'preview-pr': ("Preview a new pull request", None, 'store_true', False),
'review-pr': ("Review specified pull request", int, 'store', None, {'metavar': 'PR#'}),
'test-report-env-filter': ("Regex used to filter out variables in environment dump of test report",
None, 'regex', None),
Expand Down