-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Fix OAuthBearer OIDC preprocessor directive when using CMake without CURL #5136
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
base: master
Are you sure you want to change the base?
Conversation
🎉 All Contributor License Agreements have been signed. Ready to merge. |
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.
Pull Request Overview
This PR adds a faster “quick” test runner configuration, updates plugin support detection in tests, fixes the OAuthBearer OIDC include guard, and extends the build‐configuration checks script to support both Make and CMake inside Docker.
- Added a new quick brokerless test case in
tests/CMakeLists.txt
- Introduced runtime skip logic for unsupported
plugin.library.paths
tests intests/0066-plugins.cpp
- Switched
#ifdef WITH_OAUTHBEARER_OIDC
to#if WITH_OAUTHBEARER_OIDC
insrc/rdkafka_conf.c
- Enhanced
build-configurations-checks.sh
to accept a build tool and optional Docker image, with corresponding updates in.semaphore/semaphore.yml
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
tests/CMakeLists.txt | Added RdKafkaTestBrokerLessQuick test target |
tests/0066-plugins.cpp | Wrapped plugin.path set in a skip condition for unsupported builds |
src/rdkafka_conf.c | Changed OIDC preprocessor to #if for proper zero/one testing |
packaging/tools/build-configurations-checks.sh | Generalized to handle make or cmake modes and Docker images |
.semaphore/semaphore.yml | Invokes the updated script with explicit tool/image combinations |
@@ -69,6 +69,20 @@ static void do_test_plugin() { | |||
NULL, | |||
}; | |||
|
|||
RdKafka::Conf *conf = RdKafka::Conf::create(RdKafka::Conf::CONF_GLOBAL); | |||
if (conf->set("plugin.library.paths", | |||
"interceptor_test/interceptor_test_dummy", errstr) && |
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.
Rather than relying on the enum-to-bool conversion in if (conf->set(...))
, explicitly compare the result to RdKafka::Conf::CONF_OK
(or the appropriate error code) to make the intent clear.
"interceptor_test/interceptor_test_dummy", errstr) && | |
"interceptor_test/interceptor_test_dummy", errstr) == RdKafka::Conf::CONF_OK && |
Copilot uses AI. Check for mistakes.
errstr.find( | ||
"Configuration property \"plugin.library.paths\" not supported") != | ||
std::string::npos) { | ||
delete conf; | ||
Test::Skip( | ||
"Configuration property \"plugin.library.paths\" not supported in this " | ||
"build\n"); |
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.
[nitpick] The same error message literal is repeated twice; consider extracting it into a named constant to avoid duplication and simplify future updates.
errstr.find( | |
"Configuration property \"plugin.library.paths\" not supported") != | |
std::string::npos) { | |
delete conf; | |
Test::Skip( | |
"Configuration property \"plugin.library.paths\" not supported in this " | |
"build\n"); | |
errstr.find(PLUGIN_PATHS_ERROR) != std::string::npos) { | |
delete conf; | |
Test::Skip(tostr() << PLUGIN_PATHS_ERROR << " in this build\n"); |
Copilot uses AI. Check for mistakes.
Closes #5135