Skip to content

add support for --preview-pr #2331

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
boegel merged 13 commits into from
Oct 30, 2017
Merged

add support for --preview-pr #2331

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
ce1b274
review-pr before submitting new-pr
migueldiascosta Oct 23, 2017
792cc08
use det_easyconfig_paths
migueldiascosta Oct 24, 2017
29aea71
new option --preview-pr
migueldiascosta Oct 24, 2017
468f154
reduce code duplication
migueldiascosta Oct 24, 2017
e2b0a97
raise error if no PR # or easyconfig path is specified
migueldiascosta Oct 25, 2017
77ddd19
avoid code repetition
migueldiascosta Oct 25, 2017
79c9913
fix style
migueldiascosta Oct 25, 2017
3ba5bf9
redefine no_ec_opts and new_update_preview_pr
migueldiascosta Oct 25, 2017
a086775
use original determined paths
migueldiascosta Oct 25, 2017
f2c8bfd
fix indentation
migueldiascosta Oct 26, 2017
bde283d
shorten long line
migueldiascosta Oct 26, 2017
25e9b56
if no_ec_opts should be if any(no_ec_opts)
migueldiascosta Oct 26, 2017
ca98281
add test
migueldiascosta Oct 30, 2017
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
22 changes: 15 additions & 7 deletions easybuild/framework/easyconfig/tools.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -473,33 +473,41 @@ def find_related_easyconfigs(path, ec):

return sorted(res)


def review_pr(pr, colored=True, branch='develop'):
def review_pr(paths=None, pr=None, colored=True, branch='develop'):
"""
Print multi-diff overview between easyconfigs in specified PR and specified branch.
Print multi-diff overview between specified easyconfigs or PR and specified branch.
:param pr: pull request number in easybuild-easyconfigs repo to review
:param paths: path tuples (path, generated) of easyconfigs to review
: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 for path in fetch_easyconfigs_from_pr(pr) if path.endswith('.eb')]


if pr:
pr_files = [path for path in fetch_easyconfigs_from_pr(pr) if path.endswith('.eb')]
elif paths:
pr_files = [path[0] for path in paths]
Copy link
Member

@boegel boegel Oct 24, 2017

Choose a reason for hiding this comment

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

Hmm, shouldn't we pass the paths without the generated part down to review_pr instead?

Copy link
Member Author

@migueldiascosta migueldiascosta Oct 25, 2017

Choose a reason for hiding this comment

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

in main the paths from det_easyconfig_paths are transformed to these tuples and I used those, but I suppose I can call det_easyconfig_paths in main again, or store the original paths when it is called the first time

else:
return "no easyconfigs or pr specified"
Copy link
Member

@boegel boegel Oct 24, 2017

Choose a reason for hiding this comment

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

I would raise an error instead here, one of both mus be provided or something is wrong in the way review_pr is used...

Copy link
Member Author

@migueldiascosta migueldiascosta Oct 25, 2017

Choose a reason for hiding this comment

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

done


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 PR#%s %s has these related easyconfigs: %s" % (pr, ec['spec'], files))
if pr:
_log.debug("File in PR#%s %s has these related easyconfigs: %s" % (pr, ec['spec'], files))
else:
_log.debug("File in new PR %s has these related easyconfigs: %s" % (ec['spec'], files))
Copy link
Member

@boegel boegel Oct 24, 2017

Choose a reason for hiding this comment

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

Avoid repeating the whole thing:

if pr:
    pr_msg = "PR#%s" % pr
else:
    pr_msg = "new PR"
_log.debug("File in %s %s has these related easyconfigs: %s" % (pr_msg, 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

@boegel boegel Oct 24, 2017

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 •
edited
Loading

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

@boegel boegel Oct 25, 2017

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):
"""
Dump source scripts that set up build environment for specified easyconfigs.
Expand Down
10 changes: 6 additions & 4 deletions easybuild/main.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -276,7 +276,7 @@ def main(args=None, logfile=None, do_build=None, testing=False, modtool=None):
merge_pr(options.merge_pr)

elif options.review_pr:
print review_pr(options.review_pr, colored=use_color(options.color))
print review_pr(pr=options.review_pr, colored=use_color(options.color))

elif options.list_installed_software:
detailed = options.list_installed_software == 'detailed'
Expand Down Expand Up @@ -323,7 +323,7 @@ def main(args=None, logfile=None, do_build=None, testing=False, modtool=None):
categorized_paths = categorize_files_by_type(orig_paths)

# command line options that do not require any easyconfigs to be specified
no_ec_opts = [options.aggregate_regtest, options.new_pr, options.regtest, options.update_pr, search_query]
no_ec_opts = [options.aggregate_regtest, options.new_pr, options.preview_pr, options.regtest, options.update_pr, search_query]
Copy link
Member

@boegel boegel Oct 24, 2017

Choose a reason for hiding this comment

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

line too long, needs wrapping...

Copy link
Member

@boegel boegel Oct 24, 2017

Choose a reason for hiding this comment

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

Also, can't we use no_ec_opts or new_update_pr below, and avoid including options.new_pr, options.update_pr and options.preview_pr in no_ec_opts (since it's wrong, sematically)

Copy link
Member Author

@migueldiascosta migueldiascosta Oct 25, 2017

Choose a reason for hiding this comment

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

actually, an update_pr could just add or remove a patch, without any easyconfig in the command line...?

Copy link
Member

@boegel boegel Oct 25, 2017

Choose a reason for hiding this comment

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

OK, sure, but it doesn't make much sense for --new-pr and --preview-pr (although in theory you could also only use a patch file there)


# determine paths to easyconfigs
paths = det_easyconfig_paths(categorized_paths['easyconfigs'])
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

@boegel boegel Oct 24, 2017

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,9 @@ 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 review_pr(paths=paths, colored=use_color(options.color))
elif options.new_pr:
Copy link
Member

@boegel boegel Oct 24, 2017

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
View file
Open in desktop
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