Skip to content

fix: address kafka plugin review feedback - #19

Open
priyamkarn wants to merge 3 commits into
JordanAlec:mainfrom
priyamkarn:kafka-plugin-fixes
Open

fix: address kafka plugin review feedback#19
priyamkarn wants to merge 3 commits into
JordanAlec:mainfrom
priyamkarn:kafka-plugin-fixes

Conversation

@priyamkarn

Copy link
Copy Markdown

Summary

Adds Kafka plugin support using KafkaJS.

Changes

  • Added Kafka plugin template
  • Added produce, consume, and disconnect methods
  • Added Kafka configuration/types and environment template
  • Added Kafka example runbook following the existing plugin pattern
  • Added Redpanda Docker Compose setup for local testing
  • Added Kafka plugin documentation
  • Registered Kafka plugin in the CLI add flow
  • Added KafkaJS dependency
  • Added unit tests covering producer, consumer, disconnect, and repeated consume scenarios

Review Feedback Addressed

  • Updated example.ts to use suite().step().run("stopOnError")
  • Added missing load-env import
  • Added Kafka example under the example project
  • Added Docker Compose configuration for local testing
  • Added documentation and usage examples
  • Fixed repeated consume() calls from re-subscribing an already running consumer
  • Improved test organization and mock setup
  • Added coverage for repeated consume() calls

Validation

npm test
npm run build

@priyamkarn

priyamkarn commented Jun 18, 2026

Copy link
Copy Markdown
Author
Screenshot (259) Screenshot (255) Screenshot (256)

Screenshot (254)

/user-attachments/assets/f0753e73-c558-4016-b292-ded7fee16ee7" /> Screenshot (253)

@priyamkarn

Copy link
Copy Markdown
Author

This PR supersedes #18 and addresses all the review feedback left there.

Summary of changes made in response to the review:

  • Fixed example.ts to use the suite() factory pattern with .step() chaining and .run("stopOnError"), added missing import "../load-env.js"
  • Resolved the merge conflict in cli/add.ts
  • Fixed the double-consume runtime bug in service.ts — added a consumerStarted guard so calling consume() twice throws a clear error instead of crashing with KafkaJSNonRetriableError
  • Fixed service.test.ts — moved .mockResolvedValue() calls out of top-level declarations into beforeEach, added nested describe grouping by method, added a dedicated test for the double-consume bug and for post-disconnect reuse
  • Added example/src/services/kafka.ts, example/src/runbooks/kafka.ts, patched suite.ts and types/suite.ts so the plugin is wired into the example project and testable end-to-end
  • Added tools/kafka/docker-compose.yml (Redpanda) for local dev
  • Added documentation to example/GUIDE.md and tools/README.md
  • Updated both dtk/package-lock.json and example/package-lock.json after adding kafkajs as a dev dependency

Validation run locally:

npm test --prefix dtk     → 14 suites, 182 tests, all passed
npm run build --prefix dtk → clean
npm test --prefix example  → 13 suites, 175 tests, all passed

End-to-end runbook tested against Redpanda locally — screenshots attached.

@JordanAlec please review and approve

@JordanAlec JordanAlec left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks again for addressing the previous comments. I think we are almost there and ready to merge in. However I think your fork maybe outdated as there has been accidental removals of mongodb plugin parts.

Once addressed I'd be happy to do some final checks and get this merged in.

Appreciate your support and contribution.

Comment thread dtk/templates/plugins/kafka/README.md Outdated
Comment thread example/src/types/suite.ts
Comment thread example/src/suite.ts
Comment thread example/src/suite.ts
Comment thread example/src/suite.ts
Comment thread example/src/suite.ts
Comment thread example/.env.template
Comment thread example/package.json
Comment thread tools/README.md
@priyamkarn

Copy link
Copy Markdown
Author

@JordanAlec I Have made the required changes. Please review and approve

@priyamkarn
priyamkarn requested a review from JordanAlec June 18, 2026 23:17
@priyamkarn

Copy link
Copy Markdown
Author

@JordanAlec can you please test it and merge

@JordanAlec JordanAlec left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks for restoring the mongodb parts, confirmed those are all back (suite.ts, types/suite.ts, .env.template, package.json, tools/README.md).

One remaining accidental removal: pg has been dropped from example/package.json (and is gone from package-lock.json entirely). The SQL plugin's knex client supports client: 'pg' and needs that driver installed at runtime. This will break Postgres usage of the sql plugin even though unit tests still pass (they mock knex directly, so the missing driver isn't exercised).

Please restore pg in example/package.json dependencies. Happy to do final checks once that's back in.

@JordanAlec JordanAlec left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Correction to my last comment. Mongodb is NOT fully restored. dtk/cli/add.ts still has the mongodb entry replaced by kafka:

-  'mongodb': 'mongodb',
+  'kafka': 'kafka',

The dtk/templates/plugins/mongodb/ template files are still present and mongodb is still wired into the example project, but PLUGIN_MAP in cli/add.ts no longer has a mongodb key. Running dtk add mongodb now fails with Unknown plugin: "mongodb" and exits 1. The CLI command is broken, not just docs/lockfile. Please add 'kafka': 'kafka' alongside the existing 'mongodb': 'mongodb' entry rather than replacing it.

pg in example/package.json still needs restoring too, per my previous comment.

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