-
Notifications
You must be signed in to change notification settings - Fork 14.7k
MINOR: Add plugin.path config non-empty check in connect-plugin-path.sh #20638
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: trunk
Are you sure you want to change the base?
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 @majialoong for this patch, left some comments.
} | ||
String pluginPath = properties.getProperty(WorkerConfig.PLUGIN_PATH_CONFIG); | ||
if (pluginPath != null) { | ||
validatePluginPath(pluginPath, WorkerConfig.PLUGIN_PATH_CONFIG); |
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.
I think we should throw a ConfigException
when validating WorkerConfig.PLUGIN_PATH_CONFIG
at this stage. This would preserve the same behavior as when WorkerConfig.PLUGIN_PATH_CONFIG
is set to an empty value in the config.
"" | ||
); | ||
assertNotEquals(0, res.returnCode); | ||
assertTrue("'--plugin-path' must not be empty.\n".equals(res.err)); |
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.
Please use assertEquals
to validate the error message.
"location-a,,location-b" | ||
); | ||
assertNotEquals(0, res.returnCode); | ||
assertTrue("'--plugin-path' values must not be empty.\n".equals(res.err)); |
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.
ditto
configPath.toString() | ||
); | ||
assertNotEquals(0, res.returnCode); | ||
assertTrue("'plugin.path' must not be empty.\n".equals(res.err)); |
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.
ditto
configPath.toString() | ||
); | ||
assertNotEquals(0, res.returnCode); | ||
assertTrue("'plugin.path' values must not be empty.\n".equals(res.err)); |
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.
ditto
Thanks @m1a2st review, I have made the changes. |
KAFKA-19112 defines the
plugin.path
config as non-empty. This PR add validation forplugin.path
related config in theconnect-plugin-path.sh
tool to satisfy the same non-empty semantics, addressing the issue mentioned here.