move log.syslog to haraka-plugin-syslog #1698

Merged
merged 2 commits into from Nov 13, 2016

Conversation

Projects
None yet
2 participants
@msimerson
Member

msimerson commented Nov 10, 2016

Changes proposed in this pull request:

  • replaces plugins/log.syslog.js with haraka-plugin-syslog
  • updates haraka -h to show docs for npm packaged plugins

Checklist:

  • docs updated
  • tests updated
@smfreegard

This comment has been minimized.

Show comment
Hide comment
@smfreegard

smfreegard Nov 10, 2016

Collaborator

I'm -1 on this. IMO - syslog logging should be in core.

Collaborator

smfreegard commented Nov 10, 2016

I'm -1 on this. IMO - syslog logging should be in core.

@msimerson

This comment has been minimized.

Show comment
Hide comment
@msimerson

msimerson Nov 10, 2016

Member

Syslog is still in "core" because it's still included as a dependency. This is primarily a packaging difference, and the most significant things this affects is whether syslog unit tests are run for every Haraka PR - vs - only when changes are made to the syslog plugin.

If we can get the few things that have compile dependencies (syslog, ldap, ??) packaged as NPM modules, then we don't need to install all the build tools for every Haraka CI (travis) build. That could reduce our build times quite a bit.

Member

msimerson commented Nov 10, 2016

Syslog is still in "core" because it's still included as a dependency. This is primarily a packaging difference, and the most significant things this affects is whether syslog unit tests are run for every Haraka PR - vs - only when changes are made to the syslog plugin.

If we can get the few things that have compile dependencies (syslog, ldap, ??) packaged as NPM modules, then we don't need to install all the build tools for every Haraka CI (travis) build. That could reduce our build times quite a bit.

@smfreegard

This comment has been minimized.

Show comment
Hide comment
@smfreegard

smfreegard Nov 10, 2016

Collaborator

Syslog is still in "core" because it's still included as a dependency. This is primarily a packaging difference, and the most significant things this affects is whether syslog unit tests are run for every Haraka PR - vs - only when changes are made to the syslog plugin.

From my point of view - that isn't desirable. I'd consider syslog core functionality and therefore I want tests to run on each pull. For example - I could edit the logging hook in plugins.js and completely break all the logging plugins and I wouldn't know until I tested an install. Surely that's a bad thing...

Collaborator

smfreegard commented Nov 10, 2016

Syslog is still in "core" because it's still included as a dependency. This is primarily a packaging difference, and the most significant things this affects is whether syslog unit tests are run for every Haraka PR - vs - only when changes are made to the syslog plugin.

From my point of view - that isn't desirable. I'd consider syslog core functionality and therefore I want tests to run on each pull. For example - I could edit the logging hook in plugins.js and completely break all the logging plugins and I wouldn't know until I tested an install. Surely that's a bad thing...

@msimerson

This comment has been minimized.

Show comment
Hide comment
@msimerson

msimerson Nov 10, 2016

Member

I could edit the logging hook in plugins.js and completely break all the logging plugins and I wouldn't know until I tested an install. Surely that's a bad thing...

This plugin is a lousy example for that argument because none of the tests we have for the syslog plugin would reveal that breakage (we have only unit tests, no integration tests).

Whether this change is a bad thing depends on if you're the author of a logging plugin that's in core versus the author of a plugin that's not. If we break logging plugins, then it's best for every other plugin author that we've dog fooded it on something important like syslog, so that we get it fixed sooner and cause less harm to the more obscure logging plugins that aren't in core.

Member

msimerson commented Nov 10, 2016

I could edit the logging hook in plugins.js and completely break all the logging plugins and I wouldn't know until I tested an install. Surely that's a bad thing...

This plugin is a lousy example for that argument because none of the tests we have for the syslog plugin would reveal that breakage (we have only unit tests, no integration tests).

Whether this change is a bad thing depends on if you're the author of a logging plugin that's in core versus the author of a plugin that's not. If we break logging plugins, then it's best for every other plugin author that we've dog fooded it on something important like syslog, so that we get it fixed sooner and cause less harm to the more obscure logging plugins that aren't in core.

@msimerson msimerson changed the title from remove log.syslog to haraka-plugin-syslog to move log.syslog to haraka-plugin-syslog Nov 10, 2016

@msimerson

This comment has been minimized.

Show comment
Hide comment
@msimerson

msimerson Nov 10, 2016

Member

I could edit the logging hook in plugins.js and completely break all the logging plugins

Followup: before and after this change, we have no tests in place that would catch this type of bug. The common solutions for catching errors like this are test deployments and integration tests. The latter are much preferred because they can be [eventually] automated with Travis.

Member

msimerson commented Nov 10, 2016

I could edit the logging hook in plugins.js and completely break all the logging plugins

Followup: before and after this change, we have no tests in place that would catch this type of bug. The common solutions for catching errors like this are test deployments and integration tests. The latter are much preferred because they can be [eventually] automated with Travis.

@smfreegard

This comment has been minimized.

Show comment
Hide comment
@smfreegard

smfreegard Nov 10, 2016

Collaborator

Ok - having just chatted to baudehlo about this on IRC as well. I can see your point now. +1

Collaborator

smfreegard commented Nov 10, 2016

Ok - having just chatted to baudehlo about this on IRC as well. I can see your point now. +1

@msimerson

This comment has been minimized.

Show comment
Hide comment
@msimerson

msimerson Nov 11, 2016

Member

FYI, I'm waiting to merge this until after I've done a deployment test.

Member

msimerson commented Nov 11, 2016

FYI, I'm waiting to merge this until after I've done a deployment test.

@msimerson msimerson merged commit 0c95c02 into haraka:master Nov 13, 2016

3 checks passed

codecov/patch Coverage not affected when comparing ca6b2b4...3c9014b
Details
codecov/project 36.13% (+0.00%) compared to ca6b2b4
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@msimerson msimerson deleted the msimerson:syslog-as-npm-module branch Nov 13, 2016

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