Fix adding/removing headers in rspamd plugin; add never_add_headers #1562

Merged
merged 1 commit into from Aug 14, 2016

Conversation

Projects
None yet
3 participants
@fatalbanana
Contributor

fatalbanana commented Aug 10, 2016

Changes proposed in this pull request:

  • Oops! We had some bad documentation RE: use of task:set_rmilter_reply and consequently Haraka plugin was expecting data in a wrong format. :(
  • Also add never_add_headers option, which could be useful if you want to use custom headers from Rspamd, or just don't want to add headers for whatever reason.

Checklist:

  • docs updated
  • tests updated
@Dexus

This comment has been minimized.

Show comment
Hide comment
@Dexus

Dexus Aug 10, 2016

Member

LGTM

Von meinem iPhone gesendet

Am 11.08.2016 um 00:38 schrieb Andrew Lewis notifications@github.com:

Changes proposed in this pull request:

Oops! We had some bad documentation RE: use of task:set_rmilter_reply and consequently Haraka plugin was expecting data in a wrong format. :(
Also add never_add_headers option, which could be useful if you want to use custom headers from Rspamd, or just don't want to add headers for whatever reason.
Checklist:

[YEP] docs updated
[NOPE - N/A] tests updated
You can view, comment on, or merge this pull request online at:

#1562

Commit Summary

Fix adding/removing headers in rspamd plugin; add never_add_headers
File Changes

M docs/plugins/rspamd.md (6)
M plugins/rspamd.js (19)
Patch Links:

https://github.com/haraka/Haraka/pull/1562.patch
https://github.com/haraka/Haraka/pull/1562.diff

You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

Member

Dexus commented Aug 10, 2016

LGTM

Von meinem iPhone gesendet

Am 11.08.2016 um 00:38 schrieb Andrew Lewis notifications@github.com:

Changes proposed in this pull request:

Oops! We had some bad documentation RE: use of task:set_rmilter_reply and consequently Haraka plugin was expecting data in a wrong format. :(
Also add never_add_headers option, which could be useful if you want to use custom headers from Rspamd, or just don't want to add headers for whatever reason.
Checklist:

[YEP] docs updated
[NOPE - N/A] tests updated
You can view, comment on, or merge this pull request online at:

#1562

Commit Summary

Fix adding/removing headers in rspamd plugin; add never_add_headers
File Changes

M docs/plugins/rspamd.md (6)
M plugins/rspamd.js (19)
Patch Links:

https://github.com/haraka/Haraka/pull/1562.patch
https://github.com/haraka/Haraka/pull/1562.diff

You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

@msimerson

This comment has been minimized.

Show comment
Hide comment
@msimerson

msimerson Aug 11, 2016

Member

header settings

  • main.always_add_headers
  • main.never_add_headers
  • headers.enabled

It seems that getting the desired header behavior is...excessively complex. Maybe adding never_add_headers isn't the best choice here, and instead there should be an add_headers setting that accepts arguments: always, sometimes, never.

That should simplify both the settings and the documentation.

Member

msimerson commented Aug 11, 2016

header settings

  • main.always_add_headers
  • main.never_add_headers
  • headers.enabled

It seems that getting the desired header behavior is...excessively complex. Maybe adding never_add_headers isn't the best choice here, and instead there should be an add_headers setting that accepts arguments: always, sometimes, never.

That should simplify both the settings and the documentation.

@fatalbanana

This comment has been minimized.

Show comment
Hide comment
@fatalbanana

fatalbanana Aug 11, 2016

Contributor

If you don't mind the backwards-incompatible change we could definitely do better. Possible values for add_headers could be something like always|sometimes (default)|never|rspamd (where last case would replace headers.enabled).

Contributor

fatalbanana commented Aug 11, 2016

If you don't mind the backwards-incompatible change we could definitely do better. Possible values for add_headers could be something like always|sometimes (default)|never|rspamd (where last case would replace headers.enabled).

@msimerson

This comment has been minimized.

Show comment
Hide comment
@msimerson

msimerson Aug 11, 2016

Member

If the new config option has a different name, it should be pretty easy to write a little config setter that maps old config settings to that new option. Then the change is backwards compatible. :-)

Member

msimerson commented Aug 11, 2016

If the new config option has a different name, it should be pretty easy to write a little config setter that maps old config settings to that new option. Then the change is backwards compatible. :-)

@fatalbanana

This comment has been minimized.

Show comment
Hide comment
@fatalbanana

fatalbanana Aug 11, 2016

Contributor

Right. :) Ok. I will try cook something up. ;)

Contributor

fatalbanana commented Aug 11, 2016

Right. :) Ok. I will try cook something up. ;)

@fatalbanana

This comment has been minimized.

Show comment
Hide comment
@fatalbanana

fatalbanana Aug 11, 2016

Contributor

Well- maybe someone would like to retain Haraka's headers while adding additional headers provided by rspamd- so I did not merge governance of rspamd-provided headers into add_headers setting. Renamed headers.enabled to rmilter_headers.enabled to try create less confusion (maybe it's just worse ;)). How do you feel about it now? ;)

Contributor

fatalbanana commented Aug 11, 2016

Well- maybe someone would like to retain Haraka's headers while adding additional headers provided by rspamd- so I did not merge governance of rspamd-provided headers into add_headers setting. Renamed headers.enabled to rmilter_headers.enabled to try create less confusion (maybe it's just worse ;)). How do you feel about it now? ;)

Fix adding/removing headers in rspamd plugin
 - Also try simplify header configuration
@msimerson

This comment has been minimized.

Show comment
Hide comment
@msimerson

msimerson Aug 14, 2016

Member

👍

Member

msimerson commented Aug 14, 2016

👍

@msimerson msimerson merged commit 8980f9c into haraka:master Aug 14, 2016

2 checks passed

codecov/project 36.12% (+0.00%) compared to cdad67d
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment