No connection created when using plaintext port 5672

[derekmwright]

Attempting to leverage the library and connect to a internal testing instance of RabbitMQ and noticed via TCPDump that the client was not making the connection. It appears in the connection factory the port is being overridden to use the SSL port, even if SSL is not being specified:

https://github.com/RamblingCookieMonster/PSRabbitMq/blob/master/PSRabbitMq/New-RabbitMqConnectionFactory.ps1#L71-L72

$TcpPortProp = [RabbitMQ.Client.ConnectionFactory].GetField("Port")
$TcpPortProp.SetValue($Factory, 5671)

Perhaps it should be something like:

$TcpPortProp = [RabbitMQ.Client.ConnectionFactory].GetField("Port")
if( $Ssl ) {
  $TcpPortProp.SetValue($Factory, 5671)
} else {
  $TcpPortProp.SetValue($Factory, 5672)
}

I'll try to get something working and submit a PR but I'm not familiar with the .Net client DLL thats being leveraged here. I'm also unsure how to handle this bigger picture, ie: -Port flag for user overriding?

[RamblingCookieMonster]

Good find, thanks for the heads up!

@gpduck @gaelcolas any thoughts on this?

From my perspective, yeah, I think it would make sense to offer some more intelligent defaults (the SSL bit you added), with a parameter for overriding the port (e.g. if $Port... elseif $ssl... else... to set that port value, among other options)

Cheers!

[gaelcolas]

Agreed, adding a -Port parameter, non-mandatory, defaulting to 5672 makes sense to override the port number IMO. Then adding logic such as:

if ($Psboundparameters.containskey('ssl')) {
   $Port = 5671
}

That said, it's a breaking change.

[RamblingCookieMonster]

Sooo... How should we handle the versioning? I'm all for semantic versioning, but we're not really practicing it now, and IMHO anyone pulling from the gallery with an expectation of compatibility can and must gate to specific versions. On the other hand, wouldn't mind finally bumping this to version 1.x.x.

Any preference on this?

[gaelcolas]

Agreed on SemVer, it's a shame that the gallery does not yet support pre-releases... I'm happy for a breaking change with major version change. Bumping to 1 makes sense (I use this on production systems already anyway).

One of my goals for 2017 (but I have too many already), is to update to the RabbitMQ .Net lib version 4.x (targeting .Net core) and make PSRabbitMQ working on linux/nano... maybe this will be v2.x

[gaelcolas]

Actually, the break was made in #8 when the port got 'hard-coded' for SSL (5671). IMO That need to be fixed so that the port default to 5672 as it was before (when not specified), and setting SSL to something else than 'NONE' should override that to 5671 unless explicitly specified.

Will try to submit a PR to fix today.

[gpduck]

Fixed by #17