tavern icon indicating copy to clipboard operation
tavern copied to clipboard

Support ssl_context attributes which expose alpn

Open RFRIEDM-Trimble opened this issue 3 years ago • 8 comments

Solves #752. Still needs cleanup including unit testing new functionality. Will release from draft state when that is done. In the mean time, feel free to provide feedback on the implementation.

RFRIEDM-Trimble avatar Jan 25 '22 15:01 RFRIEDM-Trimble

I had a look and thought maybe it would be easier to add the alpn support to the mqtt library itself https://github.com/eclipse/paho.mqtt.python/pull/648 , 2 problems:

  1. It seems like nobody has really been looking at PRs or issues for a long time
  2. I'm running into issues with their CLA which I'm trying to solve

michaelboulton avatar Feb 25 '22 17:02 michaelboulton

I had a look and thought maybe it would be easier to add the alpn support to the mqtt library itself eclipse/paho.mqtt.python#648 , 2 problems:

  1. It seems like nobody has really been looking at PRs or issues for a long time
  2. I'm running into issues with their CLA which I'm trying to solve

I mean it is supported using their API when you use the set_tls_context public API, but it adds a lot of application level code as evident in my PR.

Good work getting a PR to them to make it easier, however yea, I wouldn't expect it to be as easy to get a PR approved in Paho upstream as here. I'm happy with either approach you decide. We're using it in production testing now (using my fork), and it's been stable.

RFRIEDM-Trimble avatar Feb 25 '22 20:02 RFRIEDM-Trimble

Considering the upstream PR has seen no activity, thoughts on proceeding with this approach? I'd like to get one merged so we can start tracking feature-2.0 instead of my fork.

I can fix merge conflicts here; what else would you like prior to merge?

RFRIEDM-Trimble avatar Jun 13 '22 20:06 RFRIEDM-Trimble

Considering the upstream PR has seen no activity, thoughts on proceeding with this approach? I'd like to get one merged so we can start tracking feature-2.0 instead of my fork.

I can fix merge conflicts here; what else would you like prior to merge?

Bump, please advise. I cannot keep maintain this branch with upstream; it's too much work to keep patching the merge conflicts due to all the changes in each version bump. It was clean for merge for a few days and now has conflicts again.

Anyone using AWS brokers will need this merged, so I think it's a good feature candidate.

RFRIEDM-Trimble avatar Jun 23 '22 04:06 RFRIEDM-Trimble

Sorry for not responding earlier. I started noticing that rebasing the base branch was repeatedly breaking PRs like this so I started merging instead. What I'd recommend is just

  1. Checking out the feature-2.0 branch again
  2. git cherry-pick 4d8fe27
  3. Either force push to this branch or create a new branch and new PR Other than that, It does look like paho-mqtt isn't receiving any maintenance so I agree this is the only way forward, unless tavern starts having its own subclass of the paho client.

michaelboulton avatar Jun 26 '22 10:06 michaelboulton

Sorry for not responding earlier. I started noticing that rebasing the base branch was repeatedly breaking PRs like this so I started merging instead. What I'd recommend is just

  1. Checking out the feature-2.0 branch again
  2. git cherry-pick 4d8fe27
  3. Either force push to this branch or create a new branch and new PR Other than that, It does look like paho-mqtt isn't receiving any maintenance so I agree this is the only way forward, unless tavern starts having its own subclass of the paho client.

No worries, will do!

Sorry for not responding earlier. I started noticing that rebasing the base branch was repeatedly breaking PRs like this so I started merging instead. What I'd recommend is just

  1. Checking out the feature-2.0 branch again
  2. git cherry-pick 4d8fe27
  3. Either force push to this branch or create a new branch and new PR Other than that, It does look like paho-mqtt isn't receiving any maintenance so I agree this is the only way forward, unless tavern starts having its own subclass of the paho client.

No worries. Done! Should be able to at least keep it up to date now. What do you want done so this can be merged? It's ready for merge, so I removed it from draft state.

RFRIEDM-Trimble avatar Jun 28 '22 18:06 RFRIEDM-Trimble

Hi, sorry for the late reply again - I think your branch is still off of a stale version of the feature-2.0 branch (Again, this is my fault for rebasing that branch). I created an example PR with the right git history here https://github.com/taverntesting/tavern/pull/801 , if you want me to merge that one I can do instead if you're fine with it showing as being committed by me (I understand why some people might get a bit annoyed about 'stealing' a PR though, if you just want to fix this one)

michaelboulton avatar Jul 30 '22 13:07 michaelboulton

Hi, sorry for the late reply again - I think your branch is still off of a stale version of the feature-2.0 branch (Again, this is my fault for rebasing that branch). I created an example PR with the right git history here #801 , if you want me to merge that one I can do instead if you're fine with it showing as being committed by me (I understand why some people might get a bit annoyed about 'stealing' a PR though, if you just want to fix this one)

No worries, not annoyed. I've rebased this PR on top of the latest feature-2.0 branch and signed the commit. Yea, not a huge deal either way, but since it's a non-trivial commit, I'd prefer to ensure credit stays with Trimble.

~Best. Ryan

RFRIEDM-Trimble avatar Aug 01 '22 03:08 RFRIEDM-Trimble