Skip to content
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.

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

Feature/add review pr option #1005

Merged
merged 30 commits into from
Sep 10, 2015

Conversation

nudded
Copy link

@nudded nudded commented Aug 13, 2014

use ./eb --review-pr 1017 -D | less -R to try this out.
TODO:

  • Compare with other files found in the same PR
  • add legend on top
  • add documentation / docstrings
  • figure out how to display added/removed spaces

@hpcugentbot
Copy link
Contributor

Automatic reply from Jenkins: Can I test this?

@boegel
Copy link
Member

boegel commented Aug 13, 2014

Jenkins: ok to test

@@ -858,6 +859,39 @@ def resolve_template(value, tmpl_dict):

return value

def find_relevant_easyconfigs(path, ec):
"""
Find relevant easyconfigs for ec in path based on a simple heuristic
Copy link
Member

Choose a reason for hiding this comment

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

briefly explain the heuristic used, rename function to find_relatd_easyconfigs

# along with EasyBuild. If not, see <http://www.gnu.org/licenses/>.
# #
"""
Review module for pull requests on the easyconfigs repo
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please expand the above; I was compelled to ask Ken on what is this about, so, here are some ideas:

[10:39:11] <boegel> fotis2: --review-pr 123 # will compare all easyconfigs touched in PR #123 with current develop, and give an enhanced diff report
[10:39:59] <boegel> fotis2: for each easyconfigs, it will try and find a set of closely related easyconfigs (some software + e.g. same version + toolchain name), and show the differences in a summary view

@boegel
Copy link
Member

boegel commented Aug 15, 2014

Current version can't handle easyconfigs that were deleted (which shouldn't occur, but fine):

ERROR: EasyBuild crashed with an error (at easybuild/tools/github.py:334 in
fetch_easyconfigs_from_pr): Not all patched files were downloaded to
/tmp/easybuild-FnFCKn/tmpH83hi7: ['Cube-4.2.3-goolf-1.5.14.eb', 'OPARI2-1.1.2-goolf-1.5.14.eb',
 'OTF2-1.2.1-goolf-1.5.14.eb', 'PAPI-5.2.0-goolf-1.5.14.eb', 'PDT-3.20-goolf-1.5.14.eb',
'Qt-4.8.4-goolf-1.5.14.eb', 'Score-P-1.2.3-goolf-1.5.14.eb', 'binutils-2.22-goolf-1.5.14.eb',
'null'] vs
['binutils-2.22-goolf-1.5.14.eb', 'Cube-4.2.3-goolf-1.5.14.eb', 'null', 'null',
'OPARI2-1.1.2-goolf-1.5.14.eb', 'OTF2-1.2.1-goolf-1.5.14.eb', 'PAPI-5.2.0-goolf-1.5.14.eb',
'PDT-3.20-goolf-1.5.14.eb', 'Qt-4.8.4-goolf-1.5.14.eb', 'Score-P-1.2.3-goolf-1.5.14.eb']

I'm also not sure what the nulls are about?

if not isinstance(ec, EasyConfig):
# we can safely only take the first one
easyconfigs = process_easyconfig(ec, parse_only=True)
if len(easyconfigs) > 1:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I haven't looked at the rest of the code, so maybe this is redundant:
wouldn't not ==1 do the job better, to catch the empty case?

@fgeorgatos
Copy link
Collaborator

overall good, thanks for the PR!

@boegel boegel modified the milestone: v1.16 Sep 6, 2014
@boegel boegel modified the milestones: v1.16, v2.2 Jun 24, 2015
@boegel boegel modified the milestones: v2.2, v2.3 Jul 3, 2015
@boegel boegel merged commit 46ca7e3 into easybuilders:develop Sep 10, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants