-
Notifications
You must be signed in to change notification settings - Fork 945
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
feat(noise): add WebTransport certhashes extension #4064
Conversation
@oblique I've added a |
@thomaseizinger If my GitHub profile will be shown as a contributor then I'm fine with it yes. BTW, I'm curious what is the reasoning behind this. |
@thomaseizinger To be honest, I never encountered |
Sure, no worries at all :) If you activate GitHub actions for your fork, we can achieve the same collaboration if @zvolin opens his PR with the tests against the branch in your fork! |
What I want to achieve is that we have a passing CI with the interop tests before we merge anything into master. The certhashes PR is relatively low risk though so we can probably merge that as is. |
Closing this as a result then. |
|
Hi :) @thomaseizinger I'm wondering if that won't put me in the situation which @oblique is trying to avoid. Could you help me understand what's the issue and why we can't just merge our PR's the usual way? |
Here is an example of what this looks like: 159a10b GitHub attributes the commit to all listed authors. The "issue" I am trying to solve is that I don't want to merge the webtransport implementation without a passing interop test because I don't want to those things to be an after-thought. If we merge your tests into @oblique's PR, we can achieve that. Using An alternative is to close @oblique's PR and attribute them in your PR, given that you are building on top of their commits. I don't really mind either way. Does this make sense? |
I just can't see why it would be different from getting both PR's to mergable state and then merging them one by one but I'm fine with opening a PR to @oblique |
They also semantically belong together, i.e. should be a single, atomic commit. This has additional advantages when bisecting or reverting them for example. Thank you for understanding! :) |
Description
The extension for WebTransport certhashes was added and can be configured via
Config::with_webtransport_certhashes
.In case of initiator, these certhashes will be used to validate the ones reported by responder. In case of responder, these certhashes will be reported to initiator.
Resolves #3988.
Co-authored-by: Yiannis Marangos [email protected]
Notes & open questions
This is exactly #3991 but pushed to a branch of this repository so we can stack them.
Change checklist