[FIXED] make natsOptions_SetSSLVerificationCallback experimental#860
Conversation
…WITH_EXPERIMENTAL
| arch: | ||
| type: string | ||
| default: "64" | ||
| benchmark: |
There was a problem hiding this comment.
Sorry I couldn't stand it and alphabetized the options in this PR. Unrelated.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #860 +/- ##
==========================================
+ Coverage 68.71% 70.35% +1.63%
==========================================
Files 39 47 +8
Lines 15207 15397 +190
Branches 3143 3158 +15
==========================================
+ Hits 10449 10832 +383
+ Misses 1700 1527 -173
+ Partials 3058 3038 -20 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
| #if defined(NATS_HAS_TLS) | ||
| #ifdef NATS_WITH_EXPERIMENTAL | ||
|
|
||
| // natsOptions_SetSSLVerificationCallback |
There was a problem hiding this comment.
This was in case we ever have more than a handful EXPERIMENTAL APIs, but they prob. wouldn't be grouped together anyway; will remove when I get back soon
| opts->sslCtx->callback = NULL; | ||
| } | ||
| #ifdef NATS_WITH_EXPERIMENTAL | ||
| opts->sslCtx->callback = NULL; |
There was a problem hiding this comment.
Why removing the check for skip?
There was a problem hiding this comment.
@kozlovic It was redundant; the function does not use callback when skip is on
There was a problem hiding this comment.
Yes, but silly example, but say user sets the callback first (which sets skip to false internally and sets the callback), then this API is called with skip=false, it would be a no-op, but in this case it will now set skip to false AND delete the callback...
|
(just testing it with fedora 41. should be done soon...) |
mtmk
left a comment
mtmk
left a comment
There was a problem hiding this comment.
LGTM
the only complication I can see, will be with vcpkg if we want to also publish the experimental features as well. so to me this is complicating installation a little but since vcpkg and the verification features are pretty new and possibly not being used by many it should be good until 3.11.
…cationCallback experimental (#860)
…s-io#860) * [FIXED] wrapped natsOptions_SetSSLVerificationCallback in ifdef NATS_WITH_EXPERIMENTAL * PR feedback
…cationCallback experimental (nats-io#860)
…cationCallback experimental (#860)
4 participants
Fixes #857. An alternative to @mtmk's proposal.
This new API introduced in #826 created a build issue, as @ckasabula had warned.
I propose that for now we mark it as
EXPERIMENTALand hide behind aNATS_WITH_EXPERIMENTALenv. variable as this PR implements. This will fix the immediate widespread build issue.Then I see 2 paths to fix this in a better way.
A) Go back to what @ckasabula originally proposed (+apologies!!!)
B) Leave the code "as is", but replace
NATS_HAS_TLSwithNATS_NO_TLSsince I believe it is purely an internal variable. That would alleviate the issue for all clients compiling the default TLS on. @kozlovic do you think it'd work?