Closed ChrisLynchHPE closed 7 years ago
Hi!
Yay PKI : ) Will take a peak at this, might not be until this weekend - @gpduck @gaelcolas any concerns?
Cheers!
It took a while. I really wish the Pivitol team would fix their bug with Uri and uri public properties in their RabbitMQ.Client library. Would have made this a lot easier.
I would like to help clean up the code, like add parameter sets to each parameter, so they are declared correctly, and not stepping over each other.
I love the idea! Thanks @ChrisLynchHPE for submitting!
I'll look at this in more details tomorrow.
I agree that we'll probably need some parameter sets.
For (simple) integration testing on my Windows 10 machine, I now use a linux docker container with rabbitmq, which I recommend!
docker run -d --name rabbitmq -p 5671:5671 -p 5672:5672 -p 15671:15671 -p 15672:15672 -p 25672:25672 rabbitmq:3-management
I am standing up a Windows Server 2016 Docker system to do some additional testing next week. Any update on this pull request?
Hi!
A few quick thoughts if they make sense:
Looks good to me, I'd be up for merging this in and tackling a switch to avoid the progress bar later. I'll defer to @gpduck and @gaelcolas in case they have other concerns : )
Cheers!
I'm also curious about why we roll the default TLS from 1.2 down to 1.1?
RE: Progress bar
Is $ProgressPreference enough for suppression? How do other projects handle it? I don't recall noticing other cmdlets with a parameter to turn off progress bars (but I don't see that many things with progress bars either).
I disagree about progress bars. It takes almost 5-7 seconds to initiate a connection to the SCMB and nothing is worse than seeing a blinking cursor that you have no idea what's going on. I have never seen an option to suppress Write-Progress in Cmdlets, and I certainly do not have that option in my Cmdlets I develop.
Setting the Factory Port to 5671 is just setting a default value. A user can overload it with their own parameter value if they choose. If SCMB listens on the default 5671, why wouldn't you have a default value?
@gpduck, Write-Progress is just a good thing to do, and being a good PowerShell citizen. Users want to know how things are progressing. As I stated above, I have never seen anyone have an option to suppress the output of Write-Progress. Well, only when say someone is wanting to use Write-Verbose or Write-Debug, the output can become quite "interesting".
As for the "downgrade" of TLS from 1.2 to 1.1, that was an error on my part. I was testing with my own product to make sure TLS 1.1 and 1.2 were working fine, and forgot to update it back to TLS12. If you want, I can do another PR with that updated, or after merge one of you can. Just let me know how you'd want that fixed.
Sounds good on the progress bit @ChrisLynchHPE, I'd agree with @gpduck, good as is. If folks really want to suppress it, they can use $ProgressPreference, no need worrying about adding extra code/parameters.
If you push another commit with the TLS version swapped, or just do a force push, it will update this PR, IIRC.
Cheers!
Unfortunately, I don't have a way to test Credential support, as the RabbitMQ service we use requires certificate authentication. I have yet to update your Pester test script with a framework that validates this. So all testing has been performed manually.