-
Notifications
You must be signed in to change notification settings - Fork 264
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
Allow rustls configuration in rumqttd #748
Allow rustls configuration in rumqttd #748
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR!
Removes setting the tenant id by client cert
We do need a way to specify tenant_id though, rather than removing this, we can make it optional? more on it in review comment.
can we tackle both mentioned issues in different PRs? easier to review / keep track of changes. Anyways, choice is up to you haha
Yeah, the tenant id seems like a bigger change than what this is intended for and it just unifies the way tenant id is used across all connection types. Getting this merged, and a publish to crates.io would be wonderful so that we can track against crates.io instead of our fork. Let me know if there is anything else! |
So the impetus for this PR was we need both. This now contains both changes, ready, and the other is still in draft. |
It's not just because there is other PR 😅 Separate PRs are helpful as mentioned in this comment. As these two changes are quite different, merging them in single commit won't be much helpful right. By having em separately will make it easier to undo/revert/reason about in future! wdyt? |
Its not removed. It is feature gated as requested in the previous comment if you look I made a comment saying so. I mean wanting it in separate PRs is up to you. This was free work and free contribution. I'm OK with maintaining a fork. THe code is yours do with what you want, but I won't be contributing or splitting up any more of this work. Feel free to tackle that if you want! |
I wanted to quote the separate PR part only.
I didn't mean to disrespect your/anyone's work/contribution 😕 my apologies if it came out that way! |
No disrespect taken, all good here :). I needed something fixed, so instead of just creating an issue, I submitted the changes back to the project. If you want them, feel free to merge them. They work for us, I have a fork, if they get merged cool, if not, cool. We needed rustls without client certs, we now have that. Its yours to do with what you want! |
rustls_pemfile
.Removes setting the tenant id by client cert (keeps native and rustls impls the same).
closes #745, #747
Type of change
New feature (non-breaking change which adds functionality)
Checklist:
cargo fmt
CHANGELOG.md
if it's relevant to the users of the library. If it's not relevant mention why.