Add support for github organizations #1894

timeu · 2016-08-30T08:49:49Z

An additional options build/command-line option github-org was added,
to allow users to create pull requests from an
github organization instead of a user github account (#1890).
The check_github function was modified to properly check if the user has access
to the repo in the github organization.

An additional options build/command-line option `github-org` was added, to allow users to create pull requests from an github organization instead of a user github account. The check_github function was modified to properly check if the user has access to the repo in the github organization.

timeu · 2016-08-30T08:50:09Z

I didn't add tests yet, because I am not sure how to best test this.

boegel · 2016-08-30T10:39:55Z

easybuild/tools/github.py

@@ -719,10 +719,12 @@ def _easyconfigs_pr_common(paths, ecs, start_branch=None, pr_branch=None, target
    git_repo.index.commit(commit_msg)

    # push to GitHub
-    github_user = build_option('github_user')
-    github_url = 'git@github.com:%s/%s.git' % (github_user, pr_target_repo)
+    github_user = github_account =  build_option('github_user')


github_user is no longer used in this function, so no need to define it?

boegel · 2016-08-30T10:45:35Z

@timeu it probably suffices to enhance the existing test_new_update_pr in test/framework/options.py a little bit to also use --github-org and adjust the checks accordingly

Functionality to retrieve github account was extracted into a helper function. Tests were modified to cover github-org paramter

boegel · 2016-08-30T12:26:45Z

easybuild/tools/github.py

+    """
+    if build_option('github_org'):
+        return build_option('github_org')
+    return build_option('github_user')


@timeu now begins the code style nitpicking... ;-)

We try to avoid having multiple return statements in functions, it makes it harder to grasp the code.

So, I suggest this (which is shorter too):

return build_option('github_org') or build_option('github_user')

And now that I see it like this, maybe a function is overkill? Sorry about the back-and-forth here...

yeah np ;)
So should I inline/remove the helper function ?

yeah, maybe, seems a bit too silly to have a function for a simple or...

boegel · 2016-08-30T12:27:56Z

@timeu Almost there, just some style nitpicking, thanks for looking into this!

timeu · 2016-08-30T12:36:57Z

Ok hopefully fixed all issues.
On a related note: locally the test_new_update_pr and test_new_pr_dependencies fail because the change set line does not match:

* overview of changes:
 .../easyconfigs/t/toy/toy-0.0-gompi-1.3.12-test.eb | 34 ++++++++++++++++++++++
 easybuild/easyconfigs/t/toy/toy-0.0_typo.patch     | 10 +++++++
 2 files changed, 44 insertions(+)

----------------------------------------------------------------------
Ran 1 test in 12.578s

FAILED (failures=1)

I am not sure if this is due to my local development setup or maybe a bad GitPython dependency (2.08)

boegel · 2016-08-30T13:10:41Z

@timeu hmm, I'm not sure why those tests fail for you, but I'm missing some info; can you please copy-paste the full output of the following command?

eb -O -m test.framework.options test_new

timeu · 2016-08-30T14:06:38Z

@boegel : eb -O -m test.framework.options test_new didn't work but python -O -m test.framework.options test_new did:

This is the output:

$ python -O  -m test.framework.options test_new                                                                                                                                                             
INFO: This is (based on) vsc.install.shared_setup 0.10.11
Filtered CommandLineOptionsTest tests using 'test_new', retained 3/64 tests: test_new_pr_delete, test_new_pr_dependencies, test_new_update_pr
.FF
======================================================================
FAIL: test_new_pr_dependencies (__main__.CommandLineOptionsTest)
Test use of --new-pr with automatic dependency lookup.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/net/gmi.oeaw.ac.at/nordborg/user/uemit.seren/Code/Python/easybuild/easybuild-framework/test/framework/options.py", line 2458, in test_new_pr_dependencies
    self.assertTrue(regex.search(txt), "Pattern '%s' found in: %s" % (regex.pattern, txt))
AssertionError: None is not true : Pattern '.*/foo-1\.0\.eb\s+\|\s+[0-9]+\s+\++' found in: == temporary log file in case of crash /tmp/eb-LgnJ_f/eb-eeT1P6/eb-test-00jCJ4.log
== resolving dependencies ...
== fetching branch 'develop' from https://github.com/hpcugent/easybuild-easyconfigs.git...
== copying easyconfigs to /tmp/eb-LgnJ_f/eb-eeT1P6/eb-PMAq2X/eb-7PbECU/git-working-dirS7Lj7A/easybuild-easyconfigs...

Opening pull request [DRY RUN]
* target: hpcugent/easybuild-easyconfigs:develop
* from: easybuild_test/easybuild-easyconfigs:20160830160504_new_pr_foo10
* title: "{base}[dummy/dummy] foo v1.0"
* description:
"""
(created using `eb --new-pr`)

"""
* overview of changes:
 easybuild/easyconfigs/b/bar/bar-2.0.eb | 6 ++++++
 easybuild/easyconfigs/f/foo/foo-1.0.eb | 7 +++++++
 2 files changed, 13 insertions(+)



======================================================================
FAIL: test_new_update_pr (__main__.CommandLineOptionsTest)
Test use of --new-pr (dry run only).
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/net/gmi.oeaw.ac.at/nordborg/user/uemit.seren/Code/Python/easybuild/easybuild-framework/test/framework/options.py", line 2306, in test_new_update_pr
    self.assertTrue(regex.search(txt), "Pattern '%s' found in: %s" % (regex.pattern, txt))
AssertionError: None is not true : Pattern '.*/toy-0.0-gompi-1.3.12-test.eb\s+\|\s+[0-9]+\s+\++' found in: == temporary log file in case of crash /tmp/eb-LgnJ_f/eb-Tc3nNH/eb-test-ODXh09.log
== fetching branch 'develop' from https://github.com/hpcugent/easybuild-easyconfigs.git...
== copying easyconfigs to /tmp/eb-LgnJ_f/eb-Tc3nNH/eb-UOfxlX/eb-3t3S_L/git-working-diruYhLgt/easybuild-easyconfigs...
== determining software names for patch files...
== copying patch files to /tmp/eb-LgnJ_f/eb-Tc3nNH/eb-UOfxlX/eb-3t3S_L/git-working-diruYhLgt/easybuild-easyconfigs...

Opening pull request [DRY RUN]
* target: hpcugent/easybuild-easyconfigs:develop
* from: easybuild_test/easybuild-easyconfigs:20160830160520_new_pr_toy00
* title: "{tools}[gompi/1.3.12] toy v0.0"
* description:
"""
(created using `eb --new-pr`)

"""
* overview of changes:
 .../easyconfigs/t/toy/toy-0.0-gompi-1.3.12-test.eb | 34 ++++++++++++++++++++++
 easybuild/easyconfigs/t/toy/toy-0.0_typo.patch     | 10 +++++++
 2 files changed, 44 insertions(+)

----------------------------------------------------------------------
Ran 3 tests in 37.648s

FAILED (failures=2)

boegel · 2016-08-30T14:26:59Z

@timeu Hmm, this is quite strange, I don't see why the pattern it is looking for isn't being found, since it's there... I also can't reproduce these failures on my end with your branch.

This may be a locale issue, although even then I don't see what exactly.

Can you try making these tests a little bit less strict by changing this:

diff --git a/test/framework/options.py b/test/framework/options.py
index e945e95..2f77b09 100644
--- a/test/framework/options.py
+++ b/test/framework/options.py
@@ -2297,8 +2297,8 @@ class CommandLineOptionsTest(EnhancedTestCase):
             r"^\* title: \"\{tools\}\[gompi/1.3.12\] toy v0.0\"",
             r"\(created using `eb --new-pr`\)",  # description
             r"^\* overview of changes:",
-            r".*/toy-0.0-gompi-1.3.12-test.eb\s+\|\s+[0-9]+\s+\++",
-            r".*/toy-0.0_typo.patch\s+\|\s+[0-9]+\s+\++",
+            r".*/toy-0.0-gompi-1.3.12-test.eb\s*\|",
+            r".*/toy-0.0_typo.patch\s*\|",
             r"^\s*2 files changed",
         ]
         for regex in regexs:
@@ -2448,8 +2448,8 @@ class CommandLineOptionsTest(EnhancedTestCase):

         regexs = [
             r"^\* overview of changes:",
-            r".*/foo-1\.0\.eb\s+\|\s+[0-9]+\s+\++",
-            r".*/bar-2\.0\.eb\s+\|\s+[0-9]+\s+\++",
+            r".*/foo-1\.0\.eb\s*\|",
+            r".*/bar-2\.0\.eb\s*\|",
             r"^\s*2 files changed",
         ]

timeu · 2016-08-30T15:27:47Z

Ok it works with the patch but we also need to patch the second regex block (I can add a commit for that).

I think the problem is that for some reason the changeset lines contain unicode characters. \x1b probably because the + are colored green.

.../easyconfigs/t/toy/toy-0.0-gompi-1.3.12-test.eb | 34 \x1b[1;32m++++++++++++++++++++++\x1b[m

boegel · 2016-08-30T18:24:01Z

@timeu ah, that would indeed explain it, and in that case I think the proposed change is the proper one

If you're up for adding a commit to make that change here, please do, thanks!

Relax regex because in some cases the changeset can contain unicode, due to colored "+".

timeu · 2016-08-30T20:50:17Z

Added commit to relax the regex and fix the tests

boegel · 2016-08-31T08:04:57Z

lgtm, going in, thanks for your efforts @timeu!

boegel added this to the v2.9.0 milestone Aug 30, 2016

boegel reviewed Aug 30, 2016
View reviewed changes

Added test and extracted helper function

f9a8e28

Functionality to retrieve github account was extracted into a helper function. Tests were modified to cover github-org paramter

boegel reviewed Aug 30, 2016
View reviewed changes

Fix typo and code style issues

7cce528

Fix failing tests

c26a012

Relax regex because in some cases the changeset can contain unicode, due to colored "+".

boegel merged commit b75c6b6 into easybuilders:develop Aug 31, 2016

boegel mentioned this pull request Aug 31, 2016

Support for organizations in github integration #1890

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for github organizations #1894

Add support for github organizations #1894

timeu commented Aug 30, 2016

timeu commented Aug 30, 2016

boegel Aug 30, 2016

boegel commented Aug 30, 2016

boegel Aug 30, 2016

timeu Aug 30, 2016

boegel Aug 30, 2016

boegel commented Aug 30, 2016

timeu commented Aug 30, 2016

boegel commented Aug 30, 2016

timeu commented Aug 30, 2016

boegel commented Aug 30, 2016

timeu commented Aug 30, 2016

boegel commented Aug 30, 2016

timeu commented Aug 30, 2016

boegel commented Aug 31, 2016

Add support for github organizations #1894

Add support for github organizations #1894

Conversation

timeu commented Aug 30, 2016

timeu commented Aug 30, 2016

boegel Aug 30, 2016

Choose a reason for hiding this comment

boegel commented Aug 30, 2016

boegel Aug 30, 2016

Choose a reason for hiding this comment

timeu Aug 30, 2016

Choose a reason for hiding this comment

boegel Aug 30, 2016

Choose a reason for hiding this comment

boegel commented Aug 30, 2016

timeu commented Aug 30, 2016

boegel commented Aug 30, 2016

timeu commented Aug 30, 2016

boegel commented Aug 30, 2016

timeu commented Aug 30, 2016

boegel commented Aug 30, 2016

timeu commented Aug 30, 2016

boegel commented Aug 31, 2016