Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support ssl_context attributes which expose alpn #756

Conversation

RFRIEDM-Trimble
Copy link

@RFRIEDM-Trimble RFRIEDM-Trimble commented Jan 25, 2022

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.

@michaelboulton
Copy link
Member

I had a look and thought maybe it would be easier to add the alpn support to the mqtt library itself eclipse-paho/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

@RFRIEDM-Trimble
Copy link
Author

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
Copy link
Author

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
Copy link
Author

RFRIEDM-Trimble commented Jun 23, 2022

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.

@michaelboulton
Copy link
Member

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.

@RFRIEDM-Trimble
Copy link
Author

RFRIEDM-Trimble commented Jun 28, 2022

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 RFRIEDM-Trimble marked this pull request as ready for review June 28, 2022 18:25
@michaelboulton
Copy link
Member

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)

@RFRIEDM-Trimble
Copy link
Author

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

@michaelboulton michaelboulton merged commit ae4a5b1 into taverntesting:feature-2.0 Aug 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants