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

add an additional plugin search path #1805

Merged
merged 3 commits into from Feb 6, 2017

Conversation

Projects
None yet
3 participants
@EyePulp
Collaborator

EyePulp commented Feb 5, 2017

This is to account for the module-based plugin locations when installing via NPM. They live at the same level as the Haraka/ directory (which is to say outside of it), but currently there is no lookup path in plugin_search_paths() that hits them. I think this issue isn't visible when installing from git, as npm install is run inside the Haraka directory to get all the necessary modules & plugins, which puts them in the Haraka/node_modules/ folder and matches expectations for the current list of searched paths.

Installing via NPM puts everything in this structure:

/some/path/$ npm install Haraka
# creates the following director structure

/some/path/node_modules/
/some/path/node_modules/Haraka
/some/path/node_modules/haraka-plugin-syslog
/some/path/node_modules/haraka-plugin-geoip
...etc

Changes proposed in this pull request:

  • Add an additional lookup path for the haraka-plugin- prefixed modules
add an additional plugin search path to account for the module-based …
…plugins that live in the same node_modules folder as the Haraka installed via NPM
@msimerson

This comment has been minimized.

Show comment
Hide comment
@msimerson

msimerson Feb 5, 2017

Member

Hey @EyePulp , what we already do have support for is searching for haraka-plugin-name in Haraka's installation directory (where any of Haraka's dependency npm modules will be) as well as in the haraka config directory (/etc/haraka, etc.). So if you:

cd /path/to/haraka/config
cd ..
npm install haraka-plugin-`name`

Then your plugins will get loaded as you expect and you don't have to install them with npm install -g.

Member

msimerson commented Feb 5, 2017

Hey @EyePulp , what we already do have support for is searching for haraka-plugin-name in Haraka's installation directory (where any of Haraka's dependency npm modules will be) as well as in the haraka config directory (/etc/haraka, etc.). So if you:

cd /path/to/haraka/config
cd ..
npm install haraka-plugin-`name`

Then your plugins will get loaded as you expect and you don't have to install them with npm install -g.

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Feb 5, 2017

Codecov Report

Merging #1805 into master will increase coverage by 0.18%.

@@            Coverage Diff             @@
##           master    #1805      +/-   ##
==========================================
+ Coverage   45.98%   46.17%   +0.18%     
==========================================
  Files          22       22              
  Lines        5810     5804       -6     
  Branches     1460     1456       -4     
==========================================
+ Hits         2672     2680       +8     
+ Misses       3138     3124      -14
Impacted Files Coverage Δ
plugins.js 73.52% <ø> (ø)
transaction.js 80.5% <ø> (-0.17%)
outbound.js 11.45% <ø> (+0.25%)
configfile.js 70% <ø> (+1.17%)
attachment_stream.js 22.64% <ø> (+1.21%)
mailbody.js 74.78% <ø> (+1.34%)
logger.js 81.1% <ø> (+2.36%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7481085...d4c7f84. Read the comment docs.

codecov-io commented Feb 5, 2017

Codecov Report

Merging #1805 into master will increase coverage by 0.18%.

@@            Coverage Diff             @@
##           master    #1805      +/-   ##
==========================================
+ Coverage   45.98%   46.17%   +0.18%     
==========================================
  Files          22       22              
  Lines        5810     5804       -6     
  Branches     1460     1456       -4     
==========================================
+ Hits         2672     2680       +8     
+ Misses       3138     3124      -14
Impacted Files Coverage Δ
plugins.js 73.52% <ø> (ø)
transaction.js 80.5% <ø> (-0.17%)
outbound.js 11.45% <ø> (+0.25%)
configfile.js 70% <ø> (+1.17%)
attachment_stream.js 22.64% <ø> (+1.21%)
mailbody.js 74.78% <ø> (+1.34%)
logger.js 81.1% <ø> (+2.36%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7481085...d4c7f84. Read the comment docs.

@EyePulp

This comment has been minimized.

Show comment
Hide comment
@EyePulp

EyePulp Feb 5, 2017

Collaborator

Hey @msimerson , thanks for the note and taking a look on a late Saturday night =P

I'm not sure why the haraka-plugin- modules are getting installed at all if they're not intended to be discovered by default. npm install Haraka dumps the haraka-plugin- modules in a location that isn't on any of the current plugin search paths. If a manual install of them is needed, there may be more documentation required. For instance the haraka-plugin-syslog module docs just say to add syslog to my plugins file.

I'm not married to this fix, but it does feel like it eliminates some confusion in an area that's inconsistent between NPM and git checkouts.

Collaborator

EyePulp commented Feb 5, 2017

Hey @msimerson , thanks for the note and taking a look on a late Saturday night =P

I'm not sure why the haraka-plugin- modules are getting installed at all if they're not intended to be discovered by default. npm install Haraka dumps the haraka-plugin- modules in a location that isn't on any of the current plugin search paths. If a manual install of them is needed, there may be more documentation required. For instance the haraka-plugin-syslog module docs just say to add syslog to my plugins file.

I'm not married to this fix, but it does feel like it eliminates some confusion in an area that's inconsistent between NPM and git checkouts.

@msimerson

This comment has been minimized.

Show comment
Hide comment
@msimerson

msimerson Feb 5, 2017

Member

Now I see the issue.

It looks like (from local testing) that NPM 4.x still installs Haraka's modules in (my case) ../Haraka/node_modules when installing with the -g (global) flag. However, when installing into a local path (not global) as you've shown above, npm@4 flattens the dependencies, so they're all installed in the top level node_modules directory. This is a change in npm behavior. (npm@2 for sure and I think v3 also installed a modules deps within that modules node_modules dir.)

# mkdir foo
# cd foo
# npm install Haraka

`-- Haraka@2.8.13 
  +-- address-rfc2821@1.0.1 
  +-- address-rfc2822@1.0.1 
  +-- async@2.1.4 
  | `-- lodash@4.17.4 
  +-- daemon@1.1.0 
  +-- elasticsearch@12.1.3 
  | +-- chalk@1.1.3 
  | | +-- ansi-styles@2.2.1 
 <snip>
  | `-- os-tmpdir@1.0.2 
  `-- vs-stun@0.0.7 

# ls node_modules/
.bin			dashdash		inflight		osenv
Haraka			double-ended-queue	inherits		path-is-absolute
abbrev			dtrace-provider		ipaddr.js		precond
address-rfc2821		elasticsearch		js-yaml			promise
address-rfc2822		escape-string-regexp	ldap-filter		pseudomap
ansi-regex		esprima			ldapjs			punycode
ansi-styles		extsprintf		lodash			redis
argparse		forever-agent		lru-cache		redis-commands
asap			generic-pool		maxmind			redis-parser
asn1			glob			minimalistic-assert	rimraf
asn1.js			haraka-config		minimatch		safe-json-stringify
asn1.js-rfc2560		haraka-constants	minimist		semver
asn1.js-rfc5280		haraka-net-utils	mkdirp			simple-lru-cache
assert-plus		haraka-plugin-asn	modern-syslog		sprintf-js
async			haraka-plugin-geoip	moment			strip-ansi
backoff			haraka-plugin-karma	mv			supports-color
balanced-match		haraka-plugin-limit	nan			tmp
big-integer		haraka-plugin-redis	ncp			ultron
bn.js			haraka-plugin-syslog	nopt			vasync
brace-expansion		haraka-plugin-watch	npid			verror
bunyan			haraka-results		ocsp			vs-stun
chalk			haraka-tld		once			wrappy
concat-map		haraka-utils		options			ws
core-util-is		has-ansi		os-homedir		yallist
daemon			iconv			os-tmpdir

I don't see any problems with this patch. It's probably the right thing to do in light of how npm installs dependencies now. I'm going to give @haraka/core a chance to pipe in before merging. There might be a better way.

Member

msimerson commented Feb 5, 2017

Now I see the issue.

It looks like (from local testing) that NPM 4.x still installs Haraka's modules in (my case) ../Haraka/node_modules when installing with the -g (global) flag. However, when installing into a local path (not global) as you've shown above, npm@4 flattens the dependencies, so they're all installed in the top level node_modules directory. This is a change in npm behavior. (npm@2 for sure and I think v3 also installed a modules deps within that modules node_modules dir.)

# mkdir foo
# cd foo
# npm install Haraka

`-- Haraka@2.8.13 
  +-- address-rfc2821@1.0.1 
  +-- address-rfc2822@1.0.1 
  +-- async@2.1.4 
  | `-- lodash@4.17.4 
  +-- daemon@1.1.0 
  +-- elasticsearch@12.1.3 
  | +-- chalk@1.1.3 
  | | +-- ansi-styles@2.2.1 
 <snip>
  | `-- os-tmpdir@1.0.2 
  `-- vs-stun@0.0.7 

# ls node_modules/
.bin			dashdash		inflight		osenv
Haraka			double-ended-queue	inherits		path-is-absolute
abbrev			dtrace-provider		ipaddr.js		precond
address-rfc2821		elasticsearch		js-yaml			promise
address-rfc2822		escape-string-regexp	ldap-filter		pseudomap
ansi-regex		esprima			ldapjs			punycode
ansi-styles		extsprintf		lodash			redis
argparse		forever-agent		lru-cache		redis-commands
asap			generic-pool		maxmind			redis-parser
asn1			glob			minimalistic-assert	rimraf
asn1.js			haraka-config		minimatch		safe-json-stringify
asn1.js-rfc2560		haraka-constants	minimist		semver
asn1.js-rfc5280		haraka-net-utils	mkdirp			simple-lru-cache
assert-plus		haraka-plugin-asn	modern-syslog		sprintf-js
async			haraka-plugin-geoip	moment			strip-ansi
backoff			haraka-plugin-karma	mv			supports-color
balanced-match		haraka-plugin-limit	nan			tmp
big-integer		haraka-plugin-redis	ncp			ultron
bn.js			haraka-plugin-syslog	nopt			vasync
brace-expansion		haraka-plugin-watch	npid			verror
bunyan			haraka-results		ocsp			vs-stun
chalk			haraka-tld		once			wrappy
concat-map		haraka-utils		options			ws
core-util-is		has-ansi		os-homedir		yallist
daemon			iconv			os-tmpdir

I don't see any problems with this patch. It's probably the right thing to do in light of how npm installs dependencies now. I'm going to give @haraka/core a chance to pipe in before merging. There might be a better way.

@EyePulp

This comment has been minimized.

Show comment
Hide comment
@EyePulp

EyePulp Feb 5, 2017

Collaborator

Awesome - I totally forgot npm changed to that flatter structure, probably because it's papered over by the built-in require() module just handling it. We don't have that luxury when rolling our own discovery mechanism unfortunately. Thanks npm. =)

Collaborator

EyePulp commented Feb 5, 2017

Awesome - I totally forgot npm changed to that flatter structure, probably because it's papered over by the built-in require() module just handling it. We don't have that luxury when rolling our own discovery mechanism unfortunately. Thanks npm. =)

@msimerson msimerson merged commit dbff3d2 into haraka:master Feb 6, 2017

3 checks passed

codecov/patch Coverage not affected when comparing 7481085...d4c7f84
Details
codecov/project 45.98% remains the same compared to 7481085
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

msimerson added a commit that referenced this pull request Feb 6, 2017

master -> v3 (#1812)
* More changes to try and fix unaquired socket errors

* Fix lint errors

* Fix log functions and more lint cleanups

* Another small logging fix

* Test for log methods added

* This may or may not fix issues with short atts.

More force end attempts

Finish cleaning up sending of all data

* Fix logger errors

* Don't error on re-release with non-pool sockets

* Fix error logging in test

* add an additional plugin search path to account for the module-based plugins that live in the same node_modules folder as the Haraka installed via NPM (#1805)

* replace connect.fcrdns with npm package (#1810)

* replace connect.fcrdns with npm package

* Create an outbound queue filename handler (#1792)

* create a new method for handling filenames within the outbound queue
* convert to result store module

* when proxy enabled, update remote.is_private too (#1811)

reported by golden_receiver on #haraka

msimerson added a commit that referenced this pull request Feb 6, 2017

master -> v2 (#1813)
* More changes to try and fix unaquired socket errors

* Fix lint errors

* Fix log functions and more lint cleanups

* Another small logging fix

* Test for log methods added

* This may or may not fix issues with short atts.

More force end attempts

Finish cleaning up sending of all data

* Fix logger errors

* Don't error on re-release with non-pool sockets

* Fix error logging in test

* add an additional plugin search path to account for the module-based plugins that live in the same node_modules folder as the Haraka installed via NPM (#1805)

* replace connect.fcrdns with npm package (#1810)

* replace connect.fcrdns with npm package

* Create an outbound queue filename handler (#1792)

* create a new method for handling filenames within the outbound queue
* convert to result store module

* when proxy enabled, update remote.is_private too (#1811)

reported by golden_receiver on #haraka

msimerson added a commit that referenced this pull request Feb 11, 2017

merge master to v3 (#1818)
* More changes to try and fix unaquired socket errors

* Fix lint errors

* Fix log functions and more lint cleanups

* Another small logging fix

* Test for log methods added

* This may or may not fix issues with short atts.

More force end attempts

Finish cleaning up sending of all data

* Fix logger errors

* Don't error on re-release with non-pool sockets

* Fix error logging in test

* add an additional plugin search path to account for the module-based plugins that live in the same node_modules folder as the Haraka installed via NPM (#1805)

* replace connect.fcrdns with npm package (#1810)

* replace connect.fcrdns with npm package

* Create an outbound queue filename handler (#1792)

* create a new method for handling filenames within the outbound queue
* convert to result store module

* when proxy enabled, update remote.is_private too (#1811)

reported by golden_receiver on #haraka

* fix forwarding with clinet authentication  over TLS (forward to gmail) (#1803)

* fix forwarding with client authentication  over TLS (forward to gmail etc).

msimerson added a commit that referenced this pull request Feb 11, 2017

merge master to v2 (#1819)
* More changes to try and fix unaquired socket errors

* Fix lint errors

* Fix log functions and more lint cleanups

* Another small logging fix

* Test for log methods added

* This may or may not fix issues with short atts.

More force end attempts

Finish cleaning up sending of all data

* Fix logger errors

* Don't error on re-release with non-pool sockets

* Fix error logging in test

* add an additional plugin search path to account for the module-based plugins that live in the same node_modules folder as the Haraka installed via NPM (#1805)

* replace connect.fcrdns with npm package (#1810)

* replace connect.fcrdns with npm package

* Create an outbound queue filename handler (#1792)

* create a new method for handling filenames within the outbound queue
* convert to result store module

* when proxy enabled, update remote.is_private too (#1811)

reported by golden_receiver on #haraka

* fix forwarding with clinet authentication  over TLS (forward to gmail) (#1803)

* fix forwarding with client authentication  over TLS (forward to gmail etc).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment