Skip to content
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

fix: CLI commands may be terminated with semicolon+whitespace (MINOR) #4234

Merged
merged 1 commit into from
Jan 7, 2020

Conversation

vcrfxia
Copy link
Contributor

@vcrfxia vcrfxia commented Jan 7, 2020

Description

CLI-specific commands (including run script) may be optionally terminated with a semicolon. However, the KSQL CLI doesn't currently support trailing whitespace for CLI-specific commands terminated with a semicolon, which leads to confusing behavior in some of our demos, as many of them include run script commands terminated with semicolons and newlines. For example:

ksql> RUN SCRIPT '/usr/share/doc/clickstream/clickstream-schema.sql';

(with a new line at the end) results in

Failed to read file: /usr/share/doc/clickstream/clickstream-schema.sql;
Caused by: /usr/share/doc/clickstream/clickstream-schema.sql;

since the presence of the newline causes the semicolon to fail to be stripped, and it gets attached to the end of the filename argument as a result.

This PR fixes the confusing behavior.

Testing done

Unit test + manual.

Reviewer checklist

  • Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
  • Ensure relevant issues are linked (description should include text like "Fixes #")

@vcrfxia vcrfxia requested a review from a team as a code owner January 7, 2020 00:35
Copy link
Contributor

@big-andy-coates big-andy-coates left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @vcrfxia - LGTM!

@vcrfxia vcrfxia merged commit 096b78f into confluentinc:master Jan 7, 2020
@vcrfxia vcrfxia deleted the deserialize-error-field branch January 7, 2020 19:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants