rcpt_to.routes URI format w/ LMTP support #1568

Merged
merged 6 commits into from Aug 23, 2016

Conversation

Projects
None yet
3 participants
@darkpixel
Collaborator

darkpixel commented Aug 17, 2016

  • Allow rcpt_to.routes to use URI-formatted destinations so lmtp:// vs smtp:// can be specified
  • docs updated
  • tests updated

Existing tests pass, but I'm not sure they are thorough enough. I'm a bit lost in the rcpt_to.routes.js test file, so I haven't written specific tests for my changes yet.

package.json
@@ -25,6 +25,7 @@
"js-yaml" : "~3.6.1",
"nopt" : "~3.0.4",
"npid" : "~0.4.0",
+ "url-parse" : "~1.1.3",

This comment has been minimized.

@baudehlo

baudehlo Aug 17, 2016

Collaborator

Just use node's url module, not a new npm module. It should work just fine.

@baudehlo

baudehlo Aug 17, 2016

Collaborator

Just use node's url module, not a new npm module. It should work just fine.

@@ -1,4 +1,4 @@
-# `rcpt_to.routes`
+# rcpt_to.routes

This comment has been minimized.

@msimerson

msimerson Aug 18, 2016

Member

@baudehlo, will this work now with the new html page rendering?

@msimerson

msimerson Aug 18, 2016

Member

@baudehlo, will this work now with the new html page rendering?

This comment has been minimized.

@baudehlo

baudehlo Aug 18, 2016

Collaborator

Maybe? I think so.

On Thu, Aug 18, 2016 at 11:35 AM, Matt Simerson notifications@github.com
wrote:

In docs/plugins/rcpt_to.routes.md
#1568 (comment):

@@ -1,4 +1,4 @@
-# rcpt_to.routes
+# rcpt_to.routes

@baudehlo https://github.com/baudehlo, will this work now with the new
html page rendering?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/haraka/Haraka/pull/1568/files/ec5b41f392d310c8df0401503eafc74663754164#r75331888,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAobYw_YWyyYDVEdkwLZ62SHQTK6FU4Aks5qhHvAgaJpZM4Jmyno
.

@baudehlo

baudehlo Aug 18, 2016

Collaborator

Maybe? I think so.

On Thu, Aug 18, 2016 at 11:35 AM, Matt Simerson notifications@github.com
wrote:

In docs/plugins/rcpt_to.routes.md
#1568 (comment):

@@ -1,4 +1,4 @@
-# rcpt_to.routes
+# rcpt_to.routes

@baudehlo https://github.com/baudehlo, will this work now with the new
html page rendering?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/haraka/Haraka/pull/1568/files/ec5b41f392d310c8df0401503eafc74663754164#r75331888,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAobYw_YWyyYDVEdkwLZ62SHQTK6FU4Aks5qhHvAgaJpZM4Jmyno
.

This comment has been minimized.

@darkpixel

darkpixel Aug 18, 2016

Collaborator

I didn't test it. I looked at several of the other docs and assumed that was why it showed up in the plugins list as 'Haraka' instead of 'rcpt_to.routes'. If that's not where the website generates the name, let me know and I'll change it.

@darkpixel

darkpixel Aug 18, 2016

Collaborator

I didn't test it. I looked at several of the other docs and assumed that was why it showed up in the plugins list as 'Haraka' instead of 'rcpt_to.routes'. If that's not where the website generates the name, let me know and I'll change it.

This comment has been minimized.

@baudehlo

baudehlo Aug 18, 2016

Collaborator

Yeah the regexp is horribly buggy in the site builder:

    my ($title) = ($output =~ /<h1>([^<]*)/);
    $title ||= "Haraka";

So "# plugin.name" would get turned into

plugin.name


and thus break the regexp. Gross.

I've asked for help on the site builder before but nobody comes forwards :(

Matt.

On Thu, Aug 18, 2016 at 12:24 PM, Aaron C. de Bruyn <
notifications@github.com> wrote:

In docs/plugins/rcpt_to.routes.md
#1568 (comment):

@@ -1,4 +1,4 @@
-# rcpt_to.routes
+# rcpt_to.routes

I didn't test it. I looked at several of the other docs and assumed that
was why it showed up in the plugins list as 'Haraka' instead of
'rcpt_to.routes'. If that's not where the website generates the name, let
me know and I'll change it.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/haraka/Haraka/pull/1568/files/ec5b41f392d310c8df0401503eafc74663754164#r75340896,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAobY2ymEHoJtTT9hRz6kJosSFNVxHFvks5qhIc_gaJpZM4Jmyno
.

@baudehlo

baudehlo Aug 18, 2016

Collaborator

Yeah the regexp is horribly buggy in the site builder:

    my ($title) = ($output =~ /<h1>([^<]*)/);
    $title ||= "Haraka";

So "# plugin.name" would get turned into

plugin.name


and thus break the regexp. Gross.

I've asked for help on the site builder before but nobody comes forwards :(

Matt.

On Thu, Aug 18, 2016 at 12:24 PM, Aaron C. de Bruyn <
notifications@github.com> wrote:

In docs/plugins/rcpt_to.routes.md
#1568 (comment):

@@ -1,4 +1,4 @@
-# rcpt_to.routes
+# rcpt_to.routes

I didn't test it. I looked at several of the other docs and assumed that
was why it showed up in the plugins list as 'Haraka' instead of
'rcpt_to.routes'. If that's not where the website generates the name, let
me know and I'll change it.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/haraka/Haraka/pull/1568/files/ec5b41f392d310c8df0401503eafc74663754164#r75340896,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAobY2ymEHoJtTT9hRz6kJosSFNVxHFvks5qhIc_gaJpZM4Jmyno
.

@baudehlo

This comment has been minimized.

Show comment
Hide comment
@baudehlo

baudehlo Aug 19, 2016

Collaborator

FWIW I fixed the site builder now so all the Titles should be correct.

Collaborator

baudehlo commented Aug 19, 2016

FWIW I fixed the site builder now so all the Titles should be correct.

@msimerson

This comment has been minimized.

Show comment
Hide comment
@msimerson

msimerson Aug 19, 2016

Member

Besides @baudehlo's comment on using uri to avoid a dependency (a sentiment I agree with), this LGTM.

Member

msimerson commented Aug 19, 2016

Besides @baudehlo's comment on using uri to avoid a dependency (a sentiment I agree with), this LGTM.

@baudehlo

This comment has been minimized.

Show comment
Hide comment
@baudehlo

baudehlo Aug 19, 2016

Collaborator

Yep absolutely. Fix that and I'll merge.

On Fri, Aug 19, 2016 at 6:40 PM, Matt Simerson notifications@github.com
wrote:

Besides @baudehlo https://github.com/baudehlo's comment on using uri to
avoid a dependency (a sentiment I agree with), this LGTM.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1568 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAobY6mFMSQhgRuO8G4vZSrR2_KD_dukks5qhjDFgaJpZM4Jmyno
.

Collaborator

baudehlo commented Aug 19, 2016

Yep absolutely. Fix that and I'll merge.

On Fri, Aug 19, 2016 at 6:40 PM, Matt Simerson notifications@github.com
wrote:

Besides @baudehlo https://github.com/baudehlo's comment on using uri to
avoid a dependency (a sentiment I agree with), this LGTM.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1568 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAobY6mFMSQhgRuO8G4vZSrR2_KD_dukks5qhjDFgaJpZM4Jmyno
.

@darkpixel

This comment has been minimized.

Show comment
Hide comment
@darkpixel

darkpixel Aug 23, 2016

Collaborator

Don't merge this yet. I stubbed out the replacement for url-parse, but haven't tested it in production yet. I'll hit it tomorrow.

Collaborator

darkpixel commented Aug 23, 2016

Don't merge this yet. I stubbed out the replacement for url-parse, but haven't tested it in production yet. I'll hit it tomorrow.

@darkpixel

This comment has been minimized.

Show comment
Hide comment
@darkpixel

darkpixel Aug 23, 2016

Collaborator

Tested in production. Works for me.

Collaborator

darkpixel commented Aug 23, 2016

Tested in production. Works for me.

@baudehlo baudehlo merged commit 47ae1cc into haraka:master Aug 23, 2016

2 checks passed

codecov/project 35.99% (+0.00%) compared to 7ede33a
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@darkpixel darkpixel deleted the darkpixel:rcpt-to-routes-uri branch Aug 23, 2016

@darkpixel darkpixel restored the darkpixel:rcpt-to-routes-uri branch Nov 8, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment