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

add topic prefix support #129

Merged
merged 4 commits into from Jan 19, 2017
Merged

add topic prefix support #129

merged 4 commits into from Jan 19, 2017

Conversation

jdowning
Copy link
Member

@jdowning jdowning commented Jan 17, 2017

Some users may have prefixed topic names. This commit adds support for prepending the prefix to the topic name when producing and consuming.

Additionally, the no-kafka dependency is updated and locked to 3.1.0.

Some users may have prefixed topic names. This commit adds support for
prepending the prefix to the topic name when producing and consuming.

Additionally, the `no-kafka` dependency is updated and locked to 3.1.0.
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.3%) to 86.628% when pulling 339daff on add-prefix-support into 59bd5c3 on master.

@msakrejda
Copy link
Contributor

@thody yeah, +1. And if we are moving to mainline no-kafka, we should do so in a separate PR to avoid conflating logically separate changes. Does the current no-kafka not work for you at all?

@idan idan assigned jdowning and unassigned msakrejda Jan 18, 2017
@jdowning
Copy link
Member Author

@uhoh-itsmaciek @thody We figured out the no-kafka issue on my laptop. We need to use node v6 (I was using v7). Is this 👍 to merge now?

@msakrejda
Copy link
Contributor

Ok, great. Is adding for tests reasonable? I don't recall how hairy the existing no-kafka tests were...

clientCert: config.clientCert,
clientCertKey: config.clientCertKey
cert: config.clientCert,
key: config.clientCertKey
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this change (and the same thing in the other file) still necessary? I thought this was an artifact of moving to the mainline no-kafka?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, you are correct. I will fix it soon.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 86.789% when pulling d63d2a2 on add-prefix-support into 59bd5c3 on master.

@jdowning
Copy link
Member Author

@uhoh-itsmaciek tests added 💙

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 87.156% when pulling 5b9c98d on add-prefix-support into 59bd5c3 on master.

@@ -31,6 +31,7 @@ $ heroku plugins:install heroku-kafka

For normal development, the initial setup is:
``` sh-session
# ensure node 6.x is installed
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, thanks.

@msakrejda
Copy link
Contributor

🥇 👍

@jdowning jdowning merged commit 433ec87 into master Jan 19, 2017
@jdowning jdowning deleted the add-prefix-support branch January 19, 2017 19:02
@idan idan removed the Doing label Jan 19, 2017
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

6 participants