Add support for git repositories in the 'includes' directive. #1063

pmatias · 2014-12-19T03:22:04Z

Git support for the includes directive

Objective

Be able to retrieve makefiles from git repositories

The reason

In our company we work with multiple makefiles for hundreds of different projects, and we are currently working on a better way to organise our several dozens of makefiles. One approach that we've found to be very practical for us is the following example structure:

api = 2
core = 7.x

; platform.make contains core drupal and several main modules/themes/libraries
; that we need to have available for all of our projects
includes[platform] = "platform.make"

; sub_platform.make contains specific modules/themes/libraries for
; specific requirements on a different set of projects
includes[subplatform] = "sub_platform.make"

; [...]

The problem

The only problem with this setup is that our makefiles are distributed in several private git repositories (divide and conquer: this way we can have better control on what project/patches are inside on the different types of platforms) so getting the makefiles from raw urls is off the table since it will need some kind of authentication, and we're not going to write nasty stuff to make that happen.

The solution

The 'includes' directive supports git repositories. This way, we get to assemble the main makefile the way we need it (also we get to define the core directive in the remote makefile that we want, and not the main one, giving us more control on Drupal versions).

Code proposal

With this code, drush make will support the following sentences in the makefiles:

includes[remote][makefile] = 'drupal.make'
includes[remote][download][type] = "git"
includes[remote][download][url] = "git@github.com:organisation/repository.git"
; branch could also be tag or revision, code relays on make_download_git function
includes[remote][download][branch] = "7.x"

Also, despite our reasons of needing this feature, it seems like a natural feature that 'includes' could use, since it currently supports local files, relative path local files and remote urls.

Final thoughts

The idea is to meet the requirement without going against the community standards, so if you think that we can achieve the same with another approach, or you have a different approach for the submitted code in this PR, please let me know.

Thanks!

dickolsson · 2014-12-19T11:51:36Z

commands/make/make.utilities.inc

@@ -63,7 +63,19 @@ function make_parse_info_file($makefile, $parsed = TRUE, $makefile_options = arr
          else {
            make_error('BUILD_ERROR', dt("Include file missing: !include", array('!include' => $include)));
          }
-        }
+        } elseif(!empty($include) && is_array($include)) {
+            if($include['download']['type'] = 'git'){


According to Drupal and Drush standards elseif should be on a new row and be followed by a space before (.
The if statement just below also needs a space before (.

dickolsson · 2014-12-19T11:55:10Z

I think this would be an excellent addition to the make functionality. As Pablo noted above, this would allow for a nice separated structure as well as simplifying build proceedures.

pmatias · 2014-12-19T12:11:52Z

Thanks @dickolsson for your comments!

I'm working on a proper unish test so we can check that everything's in order :)

nbouhid · 2014-12-19T12:58:14Z

commands/make/make.utilities.inc

+          if ($include['download']['type'] = 'git') {
+            $tmp_dir = make_tmp();
+            make_download_git($include['makefile'], $include['download']['type'], $include['download'], $tmp_dir);
+            $makefile = $tmp_dir . '/' .$include['makefile'];


In order to meet Drupal's coding standards, please add a space before and after string concatenations.

$makefile = $tmp_dir . '/' . $include['makefile'];

Nice catch! will fix! Thanks for pointing it!

vijaycs85 · 2014-12-19T15:51:00Z

+1 for this proposal and surely it will be big help for handling multiple projects.

weitzman · 2014-12-19T15:59:58Z

Makes sense to me. I'll leave it to @jhedstrom to evaluate code and test coverage and then merge

ergonlogic · 2014-12-19T16:17:44Z

This is an irksome limitation we've faced repeatedly.
+1

jhedstrom · 2014-12-19T18:03:38Z

This idea makes sense to me.

@pmatias are you still working on test coverage for this?

pmatias · 2014-12-20T15:19:44Z

Thank you all for your quick feedback!

@jhedstrom I've added tests/makeIncludesGitTest.php and three new makefiles under tests/makefiles, I'm not sure if it's the right way to go, but from here it looks like it's working. Please let me know if I should change something :)

Thanks!

jhedstrom · 2014-12-22T18:43:52Z

@pmatias the test is throwing a fatal error. Rather than redeclare the entire test class, you should be able to add the test method you created to the existing test class.

Also, it would be nice to add the documentation in this issue to the drush make docs.

pmatias · 2014-12-23T13:47:44Z

Thanks for pointing me to the right direction, @jhedstrom!, Now it looks better.

I would love to add this to the documentation, and I'll work today on the doc proposal.

Thanks again :)

dickolsson · 2014-12-23T14:06:34Z

tests/makefiles/includes-platform.make

+projects[drupal][version] = 7.34
+
+; Contrib Modules
+


Minor nit-pick; to be consistent with other make files we should change this to "Contrib modules" and remove the blank line under the comment.

dickolsson · 2014-12-23T16:48:43Z

tests/makefiles/includes-main.make

+includes[platform][download][url] = "https://github.com/pmatias/drush.git"
+includes[platform][download][branch] = "includes-git-support"
+
+; sub platform makefile 


More minor nit-picks; let use initial capital letter on both comments above 😄

dickolsson · 2014-12-24T07:46:30Z

This is starting to look really good, IMO. Looks like the tests are failing in the last commits. Might need to take a look at that...

@pmatias Perhaps you could squash all commits in this PR to a single commit so that it'll be cleaner for the maintainers to merge?

@jhedstrom What's the backport policy for Drush? Should we just file a separate PR once this is in to have it considered for Drush 6 as well?

@dickolsson

Now, the 'includes' directive supports the following sentences: includes[remote][makefile] = 'drupal.make' includes[remote][download][type] = "git" includes[remote][download][url] = "git@github.com:organisation/repository.git" ; branch could also be tag, code relays on make_download_git function includes[remote][download][branch] = "7.x" * Correcting code style according to Drupal and Drush standards, as pointed by @dickolsson * Correcting strange indentation after last fix * Correcting string concatenation as pointed out by @nbouhid

@jhedstrom

…irective * Moving test method for includes from git instead of having it on a separate class, as pointed by @jhedstrom * Standardising comments on makefiles based on @dickolsson's feedback. * Standardising comments as pointed out by @dickolsson

pmatias · 2014-12-24T12:21:40Z

Mm, it looks that the other tests are randomly failing, is there any way to re run the tests without triggering them via git push?

dickolsson · 2014-12-24T19:17:52Z

Yeah, the failures look random. It looks like sourceforge was down (or Travis internal DNS is having problems).

I believe only project maintainers are able to re-run tests on Travis. Or can you trigger a re-run on this @pmatias: https://travis-ci.org/drush-ops/drush/builds/45025001 ? Otherwise I think the only way is to trigger with commits (but we can squash "non-sense test trigger commits" before merge later).

…repository feature

dickolsson · 2014-12-30T08:11:30Z

docs/make.md

+      download:
+        type: "git"
+        url: "git@github.com:organisation/repository.git"
+        # Branch could be tag or revision, it relays on the standard drush git download feature.


Minor spelling mistake here. It should be "relies".

jhedstrom · 2014-12-30T16:47:23Z

This looks almost good to go-- @pmatias were you still going to add this to the docs?

pmatias · 2014-12-30T17:09:29Z

@jhedstrom I've added these lines https://github.com/drush-ops/drush/pull/1063/files#diff-17ef36930e051237472b4408a91a4f27R312
Not sure if it's the correct way though :)

Add support for git repositories in the 'includes' directive.

jhedstrom · 2014-12-31T18:05:29Z

Thanks!

pmatias · 2015-01-02T23:29:14Z

Thanks for the quick response on this PR, @jhedstrom (and everyone who provided feedback) !
One more question though, as pointed out by @dickolsson, should I create a separate PR for 6.x?

Thanks!

coveralls · 2016-07-12T23:43:06Z

Changes Unknown when pulling e3f6a14 on pmatias:includes-git-support into * on drush-ops:master*.

dickolsson reviewed Dec 19, 2014
View reviewed changes

pmatias force-pushed the includes-git-support branch from 995a1b8 to 36c3638 Compare December 19, 2014 12:41

nbouhid reviewed Dec 19, 2014
View reviewed changes

pmatias force-pushed the includes-git-support branch from e4e3ff6 to c506f3c Compare December 20, 2014 15:16

pmatias force-pushed the includes-git-support branch from 88498fe to 8f62fe5 Compare December 23, 2014 13:16

dickolsson reviewed Dec 23, 2014
View reviewed changes

pmatias force-pushed the includes-git-support branch from 98e7cb3 to 214483f Compare December 23, 2014 14:18

dickolsson reviewed Dec 23, 2014
View reviewed changes

Pable Fabregat added 2 commits December 24, 2014 10:02

pmatias force-pushed the includes-git-support branch from 0bace5d to cfc5e1f Compare December 24, 2014 12:04

pmatias force-pushed the includes-git-support branch 4 times, most recently from 7e83140 to bc3ee94 Compare December 29, 2014 14:29

Adding corresponding documentation to new download makefile from git …

5e85738

…repository feature

pmatias force-pushed the includes-git-support branch from bc3ee94 to 5e85738 Compare December 29, 2014 17:25

weitzman added the area-make label Dec 29, 2014

weitzman assigned jhedstrom Dec 29, 2014

dickolsson reviewed Dec 30, 2014
View reviewed changes

Correcting spelling mistake as pointed out by @dickolsson

e3f6a14

jhedstrom added a commit that referenced this pull request Dec 31, 2014

Merge pull request #1063 from pmatias/includes-git-support

e9024dd

Add support for git repositories in the 'includes' directive.

jhedstrom merged commit e9024dd into drush-ops:master Dec 31, 2014

pmatias added a commit to pmatias/drush that referenced this pull request Jan 5, 2015

Backport of drush-ops#1063 for the 6.x branch

127a9dc

pmatias mentioned this pull request Jan 5, 2015

[Backport] Add support for git repositories in the 'includes' directive. #1081

Merged

pmatias added a commit to pmatias/drush that referenced this pull request Mar 2, 2015

Backport of drush-ops#1063 for the 6.x branch

b48c2b3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for git repositories in the 'includes' directive. #1063

Add support for git repositories in the 'includes' directive. #1063

pmatias commented Dec 19, 2014

dickolsson Dec 19, 2014

dickolsson commented Dec 19, 2014

pmatias commented Dec 19, 2014

nbouhid Dec 19, 2014

pmatias Dec 19, 2014

vijaycs85 commented Dec 19, 2014

weitzman commented Dec 19, 2014

ergonlogic commented Dec 19, 2014

jhedstrom commented Dec 19, 2014

pmatias commented Dec 20, 2014

jhedstrom commented Dec 22, 2014

pmatias commented Dec 23, 2014

dickolsson Dec 23, 2014

dickolsson Dec 23, 2014

dickolsson commented Dec 24, 2014

pmatias commented Dec 24, 2014

dickolsson commented Dec 24, 2014

dickolsson Dec 30, 2014

jhedstrom commented Dec 30, 2014

pmatias commented Dec 30, 2014

jhedstrom commented Dec 31, 2014

pmatias commented Jan 2, 2015

coveralls commented Jul 12, 2016 •

edited

Loading

Add support for git repositories in the 'includes' directive. #1063

Add support for git repositories in the 'includes' directive. #1063

Conversation

pmatias commented Dec 19, 2014

Git support for the includes directive

Objective

The reason

The problem

The solution

Code proposal

Final thoughts

dickolsson Dec 19, 2014

Choose a reason for hiding this comment

dickolsson commented Dec 19, 2014

pmatias commented Dec 19, 2014

nbouhid Dec 19, 2014

Choose a reason for hiding this comment

pmatias Dec 19, 2014

Choose a reason for hiding this comment

vijaycs85 commented Dec 19, 2014

weitzman commented Dec 19, 2014

ergonlogic commented Dec 19, 2014

jhedstrom commented Dec 19, 2014

pmatias commented Dec 20, 2014

jhedstrom commented Dec 22, 2014

pmatias commented Dec 23, 2014

dickolsson Dec 23, 2014

Choose a reason for hiding this comment

dickolsson Dec 23, 2014

Choose a reason for hiding this comment

dickolsson commented Dec 24, 2014

pmatias commented Dec 24, 2014

dickolsson commented Dec 24, 2014

dickolsson Dec 30, 2014

Choose a reason for hiding this comment

jhedstrom commented Dec 30, 2014

pmatias commented Dec 30, 2014

jhedstrom commented Dec 31, 2014

pmatias commented Jan 2, 2015

coveralls commented Jul 12, 2016 • edited Loading

coveralls commented Jul 12, 2016 •

edited

Loading