Outbound split and move into folder #1850
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
There's still more to do on this branch. Just wanted the PR in place first. |
|
@msimerson @gwieshammer outbound with the splitting now works but the tests fail. The outbound tests are a bit of a twisty maze of passages that I'm really unfamiliar with - you two have done all the previous work on them - any chance one of you could take a peek? |
|
I've wandered in that maze once or twice and I always come out horrified. If I were doing outbound, I'd nuke them tests and just start over. Write some unit tests that verify that each class/file (config, HmailItem, pool, etc) behaves mostly as you expect, and you'll have much better than we've got now. |
hatsebutz
reviewed
Apr 18, 2017
outbound/hmail.js
Outdated
| var mx_lookup = require('./mx_lookup'); | ||
| var outbound = require('./index'); | ||
|
|
||
| var get_tls_options = require('./_get_tls_options'); |
There was a problem hiding this comment.
Can not see the file _get_tls_options - any hints?
|
Sorry about that - must have forgot to add it. I'll try and do it tonight.
…On Tue, Apr 18, 2017 at 5:21 AM, Gerhard Wieshammer < ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In outbound/hmail.js
<#1850 (comment)>:
> @@ -23,6 +23,10 @@ var client_pool = require('./client_pool');
var cfg = require('./config');
var _qfile = require('./qfile');
var queuelib = require('./queue');
+var mx_lookup = require('./mx_lookup');
+var outbound = require('./index');
+
+var get_tls_options = require('./_get_tls_options');
Can not see the file _get_tls_options - any hints?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1850 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAobYzXXgzEhBmiYAf7mp91Ru-czk1G2ks5rxICKgaJpZM4MY0pD>
.
|
|
I've added that file and fixed a test. Still failing. Is node_unit just hiding the real error somewhere? Any way to get it to be more verbose? |
You are experiencing the joy that is test/fixtures/vmharness, aka vm.runInNewContext, and the hell it imposes on anyone attempting to debug code that's run in other contexts. You're experiencing why I'm such an advocate for getting rid of it. |
|
How can I atone for this sin I committed? ;)
…On Tue, Apr 18, 2017 at 7:15 PM, Matt Simerson ***@***.***> wrote:
Is node_unit just hiding the real error somewhere? Any way to get it to be
more verbose?
You are experiencing the joy that is test/fixtures/vmharness, aka
vm.runInNewContext, and the hell it imposes on anyone attempting to debug
code that's run in *other* contexts. You're experiencing why I'm such an
advocate for getting rid of it.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1850 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAobYw7dSy_dVYFgCS1ws8nHIQlE9sssks5rxUQHgaJpZM4MY0pD>
.
|
|
I've converted 2 of the tests using vm_harness for Bounce-Triggering into plain node_unit. How to get them included here, or should I just make a new PR? |
|
@hatsebutz, you can create a PR against this branch (outbound-split). |
|
PR #1889 against outbound-split reworks the failing tests from outbound_protocol from using vm_harness to plain node_unit. I've moved them out of the folder one level up and renamed them. |
msimerson
force-pushed
the
outbound-split
branch
from
bc03448 to
b9054eb
Compare
|
To fix the outbound.js conflicts that prevented merging, I have rebased this against master. I also added a commit fixing the path to fsync_writestream. Because outbound is already broken in master, I'm +1 on merging this and getting it some wider testing. |
Codecov Report
@@ Coverage Diff @@
## master #1850 +/- ##
==========================================
+ Coverage 50.24% 58.09% +7.84%
==========================================
Files 22 30 +8
Lines 5784 5858 +74
Branches 1458 1461 +3
==========================================
+ Hits 2906 3403 +497
+ Misses 2878 2455 -423
Continue to review full report at Codecov.
|
msimerson
approved these changes
Apr 19, 2017
|
There's a few things that are out of date in it. I need to bring it up to where outbound.js currently is in master first. Unfortunately as discussed before, that's a manual process. |
|
I realize it's not quite done, but to make it easier for baudehlo to progress, and b/c outbound in master is already broken, I'm merging. |
|
OK and it's a good prod for me anyway... Now I just have to figure out how
to get the outbound.js just before this merge ;)
…On Thu, May 4, 2017 at 2:59 PM, Matt Simerson ***@***.***> wrote:
Merged #1850 <#1850>.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1850 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAobYzMrocaDBCzgK8a1QwBWKUhxme-oks5r2iAugaJpZM4MY0pD>
.
|
Easy, github file history browser. |
msimerson
added a commit
that referenced
this pull request
Jul 29, 2017
* ignore undefined socket.remoteAddress (#1846) similar to #239 fixes #1835 * URL to manual was 404, point to Plugins.md (#1844) * smtp forward dest split routing (#1847) * split transactions with different forward routing * use tls_ini_section_with_defaults (#1848) * use net_utils.tls_ini_section_with_defaults() fixes #1691 * assure conn/tran still exists before storing results (#1849) * chore(package): update redis to version 2.7.0 (#1854) https://greenkeeper.io/ * fix(package): update ipaddr.js to version 1.3.0 (#1857) https://greenkeeper.io/ * Fix modifier regexp (#1859) * doc typo in config file name (#1865) Actual this is typo in plugin source - it uses "ini" file from another plugin called rabbitmq . So, in fact, no one of the plugins uses config file with name "rabbitmq_amqplib.ini" . This cause error (default values for the plugin) if you trying use the official documentation. * fix(package): update async to version 2.2.0 (#1863) https://greenkeeper.io/ * replace util.inherits with class .. extends (#1862) * replace the discouraged util.inherits with `class Foo extends Bar` syntax * chunkemitter: remove Buffer.concat shim for node < 0.7.11 * fsync_writesteam: moved into ./outbound (only required once) * tls_socket: always set `isServer=true` (an recent merge made it conditional) * removed test for cleartext.getCipher, it *should* always be present * Added RabbitMQ vhost support (#1866) * Add RabbitMQ vhost support Virtual Host. Start with "/". Leave blank or not use if you don't want to use virtual hosts. * Get pool (#1868) * pass cfg object instead, simplifying get_pool * more consistency with variable naming and passing * add integration test: send message w/smtp_client * tls consistency cleanups (#1851) * tls_socket: add options.SNICallback - refresh options.secureContext after setting a different cert in options - plugins/tls: add plugin.loadPemDir - replace some + ‘ ‘ + patterns with string interpolation - replace some `function (a,b) {` patterns with `(a,b) => {` * add very basic tests for tls_socket * move SNI into plugins/tls * revert * tls: remove interim variables (#1871) * - remove interim variables * remove unused _controlReleased * Split delivery relay (#1875) * do not defer relaying clients with split transactions * disable naïve comment stripping (#1876) * disable naïve comment stripping fixes #1874 * tickle travis * fix(package): update async to version 2.3.0 (#1878) https://greenkeeper.io/ * chore(package): update ocsp to version 1.2.0 (#1879) https://greenkeeper.io/ * Added missing 'default' keyword in rspamd plugin. Score wasn't added … (#1856) * Added missing 'default' keyword in rspamd plugin. Score wasn't added properly to header * Logging envelope from / recipients in lowercase in order to avoid changing ES analyzer ( slower performance ) * handle case where OCSP server is unavailable (#1880) this likely fixes #1855 * Add .editorconfig (#1884) * Removing typo from 'relay.md' (#1886) * Fix for issue #1410 (#1887) * Fix for issue #1410 * Remove logging * Confirmed that it works * fixed auth_proxy socket error (#1894) #1893 * Implement the missing upgrade method on SMTPClient. (#1901) * fix(package): update async to version 2.4.0 (#1896) https://greenkeeper.io/ * 1861 binary log (#1902) * Binary log file The log file without that replacement, is interpreted as binary file log issue. * Binary log replace regexp binary replace with code replace * remove variable, line wrap closes 1861 * Outbound split and move into folder (#1850) * Move outbound to its own sub-folder * Start of splitting up outbound into separate files * Progress on splitting * More progress, not working yet * Outbound now seems to work. Tests still fail. * Add missing file * Fix outbound.js test * Fix lint errors * Outbound-Tests for RFC3464 bounce messages reworked (#1889) * fix path to fsync_writestream (./ vs ../) * make sure list_re is defined before access (#1903) * Check error code instead of number. Fixes GH-1895. (#1899) * Bring client pool up to where we were before split (#1915) * Bring client pool up to where we were before split * Add changes from #1848 * Merge in class/extends changes * Fix all logging to be [outbound] prefixed * Fix self logger * Fix net_utils call * fix(package): update sprintf-js to version 1.1.0 (#1906) * F/outbound client certificates (#1908) * Add tls_options and associated tests to queue/smtp_forward * Fix exception when no tls blacklist added. Add tests for smtp_client. * Add documentation for outbound client_certificate * Fix path parsing bug on Windows platform (#1919) * Attempt to add some belt and braces around FIN (#1916) * Attempt to add some belt and braces around FIN * Add logging on write failure * Allow outbound pools to be disabled (#1917) * Allow outbound pools to be disabled * Few changes after actually testing the code * Fix for shutting down clients * Set graceful shutdown off by default (#1927) * Set graceful shutdown off by default Still allow haraka -c <path> --graceful Also sets graceful shutdown to shutdown in max 2xChildren amount of time it sets to shutdown. * Allow gracefulShutdown() to always be graceful Makes the tests pass * Fix comment * Run dumped logs through plugins not console (#1929) * Don't blow the stack on qstat (#1930) * Don't blow the stack on qstat * Should fix another call stack blowing issue * Allow "Unknown Result" and Socket Error to Try Next Host (#1931) * Allow "Unknown Result" and Socket Error to Try Next Host If multiple hosts are listed for clamd and an "unknown result" is returned or a socket error occurs after connection, then attempt to try remaining hosts before returning DENYSOFT. * Remove trailing spaces * Clear off some done tasks (#1928) * Clear off some done tasks * remove completed or abandoned TODO items * Check pool exists before delete (#1937) * Fixes loading outbound under cluster. (#1934) I don't really understand why this stuff broke. It seems to be a bug in node's require() caching, frankly. When loading modules they would return {} before actually being loaded. This appears to be due to recursive requires. This might fix #1933 Please test * Fix cluster messaging for node v6+ (#1938) * Fix undefined variable platformDOT in hmail.js (#1943) * Update qfile.js * Update hmail.js * Update index.js * Fix queue not loaded for single process (#1941) * Fix queue not loaded for single process * Update server.js * Use the sunset keyword in version specific code blocks (#1939) Because `sunset` is the keyword we'll grep for (if we follow our instructions) when doing a semver major release. * Fix PROTOCOL logs that have intermediate \n chars (#1947) Most noted in SpamAssassin output * Load logger in a setImmediate call (#1948) * Bump haraka-results required version (#1949) * Bump haraka-results required version * Update package.json * Fix undefined FsyncWriteStream var (#1953) * Fix undefined FsyncWriteStream var * Update hmail.js * Be more strict in attachment filename matching (#1957) * Be more strict in attachment filename matching This copes with filename="d'euvre" * Add a simple test * Use punycode domain (#1944) * Support SMTPUTF8 properly * Use punycode for mail from too * Enable SMTPUTF8 support * Transaction is null there. Wait until it's set * Add needed utf8 changes to connection.js * punycode for the other addresses * Fix trailing space * Be consistent with todo.domain (#1960) * Minor typo fix (#1963) Thanks * Update package.json (#1968) * Fix unaquired socket errors in test suite (#1971) * Support the new address-rfc2822 (#1970) * Support the new address-rfc2822 New module supports Groups, which have no host method * Force version of rfc2822 required by the fix * Fix missing backslash * RabbitMQ: fix encoding of user and password when creating connection string (#1964) * fix(package): update ipaddr.js to version 1.4.0 (#1972) * Fix link redirection (#1975) * require node LTS version (6+) (#1958) * require node LTS version (6+) * appveyor: install node v6 * whitespace changes for eslint 4 compat (#1979) * whitespace changes for eslint 4 compat * one more ws change * fix(package): update iconv to version 2.3.0 (#1981) * fix(package): update async to version 2.5.0 (#1982) * Add node v8 (#1951) * Add node v8 * enable node v8 testing, permitting v8 failures * import Plugins.md from v3 branch (#1991) * remove logging line (#1985) the prefix is not required in config/plugins as the message states, but it is required for an NPM packaged plugin that requires another NPM packaged plugin and is then spurious * doc: add note that smtp_forward only supports STARTTLS (#1988) * Update qmail-queue.js (#1997) qmail-queue binary Adds a Received: header with bare LF. Haraka sets CRLF by default resulting in mixed line endings which break header processing in some e-mail clients. messagestream can convert CRLF to LF on the fly when calling qmail-send. * rebuild blacklist upon file change (#1990) previously just added new entries fixes 1987 * Remove spurious logs (#1989) * replace plugins/log.elasticsearch with npm packaged (#2004) * replace access with npm packaged haraka-plugin-access (#1992) * use WRITE_EXCL from haraka-constants (#2011) * fix(package): update js-yaml to version 3.9.0 (#2002) * build_todo() is part of the outbound/index.js api (#2016) * rename xclient.hosts to match plugin & docs (#2014) fixes #1265 * correct the config file name to relay.ini (#2012) * fix(package): update semver to version 5.4.0 (#2015) * require node 8 tests to pass (#2017) now that node 8.2+ is on Travis * fixes #1765 (#2013) * Fix auth plugin failure when re-selecting auth method (#2000) When sending the following sequence of commands, the auth plugin fails since it tries to decode an undefined value as base64: AUTH LOGIN Z2lyaXNo 334 UGFzc3dvcmQ6 AUTH LOGIN 500 Unrecognized command # at this point plugin has failed Fixes #1999 * Improve logging UUID tracking in Outbound (#2018) * prep release 2.8.14 (#1932) * prep for next release * update Changes * improve Changes formatting * make the rest of the versions h2 * update Changes
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #1831
Changes proposed in this pull request:
Checklist: