normalized connection properties #1547

Merged
merged 3 commits into from Oct 3, 2016

Conversation

Projects
None yet
3 participants
@msimerson
Member

msimerson commented Jul 21, 2016

work in progress

Fixes #1098

connection.js changes only in #1577

  • converts connection.remote_* to connection.remote.*
  • converts connection.local_* to connection.local.*
  • converts connection.using_tls -> connection.tls.enabled
  • converts connect.greeting -> connection.hello.verb
  • maintains previous names, providing backwards compatibility

Changes proposed in this pull request:

  • updates all code to use the new normalized locations
  • Updates all the connection.set invocations to use the new connection.set()

Checklist:

  • docs updated
  • tests updated
@baudehlo

This comment has been minimized.

Show comment
Hide comment
@baudehlo

baudehlo Jul 21, 2016

Collaborator

This is going to break basically every application built on Haraka, so it needs some careful consideration. I know it'll break the Haraka mailing list, and emailitin, for example.

One way to mitigate some of that might be to do it in a backwards compatible way (have both sets of data at first). Ugly but doable.

Collaborator

baudehlo commented Jul 21, 2016

This is going to break basically every application built on Haraka, so it needs some careful consideration. I know it'll break the Haraka mailing list, and emailitin, for example.

One way to mitigate some of that might be to do it in a backwards compatible way (have both sets of data at first). Ugly but doable.

@msimerson

This comment has been minimized.

Show comment
Hide comment
@msimerson

msimerson Jul 23, 2016

Member

Look at the patch, it already does it in a backwards compatible way. :-) at least mostly. I ran out of time. It needs some more review before I can confidently say 100%.

Member

msimerson commented Jul 23, 2016

Look at the patch, it already does it in a backwards compatible way. :-) at least mostly. I ran out of time. It needs some more review before I can confidently say 100%.

@smfreegard

This comment has been minimized.

Show comment
Hide comment
@smfreegard

smfreegard Jul 23, 2016

Collaborator

Look at the patch, it already does it in a backwards compatible way. :-) at least mostly. I ran out of time. It needs some more review before I can confidently say 100%.

If that is true, then why not split this patch into two. If you modify just the connection properties and you've correctly done it in a backwards-compatible way, then all the existing tests and integration should pass - that way you know you've done it right.

Collaborator

smfreegard commented Jul 23, 2016

Look at the patch, it already does it in a backwards compatible way. :-) at least mostly. I ran out of time. It needs some more review before I can confidently say 100%.

If that is true, then why not split this patch into two. If you modify just the connection properties and you've correctly done it in a backwards-compatible way, then all the existing tests and integration should pass - that way you know you've done it right.

@baudehlo

This comment has been minimized.

Show comment
Hide comment
@baudehlo

baudehlo Jul 23, 2016

Collaborator

+1

On Jul 23, 2016, at 2:49 AM, Steve Freegard notifications@github.com wrote:

Look at the patch, it already does it in a backwards compatible way. :-) at least mostly. I ran out of time. It needs some more review before I can confidently say 100%.

If that is true, then why not split this patch into two. If you modify just the connection properties and you've correctly done it in a backwards-compatible way, then all the existing tests and integration should pass - that way you know you've done it right.


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

Collaborator

baudehlo commented Jul 23, 2016

+1

On Jul 23, 2016, at 2:49 AM, Steve Freegard notifications@github.com wrote:

Look at the patch, it already does it in a backwards compatible way. :-) at least mostly. I ran out of time. It needs some more review before I can confidently say 100%.

If that is true, then why not split this patch into two. If you modify just the connection properties and you've correctly done it in a backwards-compatible way, then all the existing tests and integration should pass - that way you know you've done it right.


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

connection.js
+ self.remote.ip = src_ip;
+ self.remote.port = parseInt(src_port, 10);
+ self.remote.host = null;
+ self.hello.host = null;

This comment has been minimized.

@baudehlo

baudehlo Jul 23, 2016

Collaborator

Surely these should use .set()?

@baudehlo

baudehlo Jul 23, 2016

Collaborator

Surely these should use .set()?

connection.js
+ cipher: {},
+ authorized: null,
+ }
+

This comment has been minimized.

@baudehlo

baudehlo Jul 23, 2016

Collaborator

Wouldn't it be better to use .set() here too?

@baudehlo

baudehlo Jul 23, 2016

Collaborator

Wouldn't it be better to use .set() here too?

This comment has been minimized.

@msimerson

msimerson Sep 3, 2016

Member

Better for whom? Surely having a bunch of function calls is not going to be faster than just declaring the object and a few properties. I don't think that using .set() makes the code more readable for humans. In what way do you think it'd be better?

@msimerson

msimerson Sep 3, 2016

Member

Better for whom? Surely having a bunch of function calls is not going to be faster than just declaring the object and a few properties. I don't think that using .set() makes the code more readable for humans. In what way do you think it'd be better?

msimerson added some commits Jul 21, 2016

update usage of normalized props repo wide
* updated using_tls -> tls.enabled
* backwards compat for remote_info
* set backwards compat greeting
* normalized connection properties
* add tests to assure legacy + normalized both present
* update docs/Connection
move legacy locations into inline comments
for a handy reference chart of old <-> new settings

@msimerson msimerson merged commit e1ec428 into haraka:master Oct 3, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@msimerson msimerson deleted the msimerson:1098-normalized-connection-properties branch Oct 3, 2016

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