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: Append state.dir directive to ksql-server.properties #7003

Merged
merged 1 commit into from
Mar 17, 2021

Conversation

andrewegel
Copy link
Contributor

  • Moved RPM scriptlet that appends state.dir to %post
  • Appended newline to config/ksql-server.properties so the append above
    doesn't wind up on the same line as another directive

Description

The debian packages' /etc/ksqldb/ksql-server.properties had this line incorrectly appended

# ksql.schema.registry.url=http://localhost:8081state.dir=/var/lib/kafka-streams

TO remedy that, I added an extra newline to config/ksql-server.properties where I believe this file is sourced from.

And the RPMs /etc/ksqldb/ksql-server.properties didn't have this at all. RPM didn't because the script (that I moved) Was running before the package was even installed, so the file -f check failed. I moved this check to %post so that the check/append occurs when the package & file are actually installed.

Testing done

Non-code change. Nightly builds will pick this change up where we can verify the change is present in the OS packages.

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 #")

- Moved RPM scriptlet that appends state.dir to %post
- Appended newline to config/ksql-server.properties so the append above
doesn't wind up on the same line as another directive
@andrewegel andrewegel requested a review from a team as a code owner February 11, 2021 22:04
@andrewegel
Copy link
Contributor Author

@confluentinc/ksql this is still pending review from one of you - minor change.

Copy link
Member

@spena spena left a comment

Choose a reason for hiding this comment

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

lgtm

@andrewegel andrewegel merged commit 4893e90 into master Mar 17, 2021
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.

2 participants