-
-
Notifications
You must be signed in to change notification settings - Fork 647
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
KsqlDB support #1324
KsqlDB support #1324
Conversation
@dweber019 amazing works !!! 👏 👏 👏 |
No worries take your time. |
697d263
to
f5f140e
Compare
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.
Really amazing work 👏
I would address the package lock comment before merging (not my call of course).
One small thing: I had to run prettier manually on most files (the build should fail now since eslint should consider those errors). There's a task npm run prettier:fix
for that.
In general, it's probably easier to integrate prettier in the editor you're using (so you don't run into this).
I did some minimal testing (I'm also not a ksqlDB user so hard for me to do extensive tests) and everything I tried works great!
const { clusterId, ksqlDBId, selectedTab } = this.state; | ||
const { history, match, location } = this.props; | ||
|
||
switch (selectedTab) { |
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.
If you introduce a little mapping like:
const tabs = {
info: KsqlDBInfo,
streams: KsqlDBStreams,
tables: KsqlDBTables,
queries: KsqlDBQueries
};
Then you can get rid of the switch all together like this:
const SelectedTab = tabs[selectedTab] || KsqlDBStreams;
return (
<SelectedTab
clusterId={clusterId}
ksqlDBId={ksqlDBId}
history={history}
match={match}
location={location}
/>
);
Probably a bit of a personal preference but I think the version with the map leads to a little more maintainable code (and it's a little shorter too)
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 tried to keep the code style the same as in the Kafka Connect implementation but your suggestion is much nicer!
Thx
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 would have done the same! I'll go over the kafka connect code soon-ish and see what I can do
roles: JSON.parse(sessionStorage.getItem('roles')), | ||
}; | ||
|
||
tabs = ['streams', 'tables', 'queries', 'info']; |
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.
this could be derived from the keys of the tab mapping I'm suggesting in the next comment
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.
Used Object.keys(...) for it.
@@ -0,0 +1,83 @@ | |||
import React from 'react'; |
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.
Not sure how to comment on the package lock file line... (sorry)
In the current dev branch we added a nmvrc so you can use nvm to select the "right" node/npm version.
This should get rid of the file change all together (or lead to a much smaller change)
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.
should be the same as I haven't changed anything. I updated my dev branch.
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.
maybe something in the way we run the app in dev? I'll investigate.
Updated PR. |
@dweber019: have you still some things to do on this one? or review is required ? |
On my side it good to merge. |
@dweber019 not my call (I'm just contributing little things here and there) but if it were up to me I'd revert back the change to the package lock (as I understand it, there's no need for any changes to the file anyway) before merging. As for the rest, it looks good to me (but I'd take it with a grain of salt since I could only do some trivial testing) |
Reverted the package lock. |
amazing work @dweber019 !! Really appreciate, let people try and send feedback 👍 |
As stated in #115 it would be nice to have a KsqlDB integration.
If have a first contribution working. I think it should be extended in the future as there are more awesome feature to implement but for now the following features are implemented:
Here some impressions, starting with the lists
![akhq-streams](https://user-images.githubusercontent.com/1021324/210272484-05999614-275d-4209-97e0-f40b6a02033a.png)
![akhq-tables](https://user-images.githubusercontent.com/1021324/210272504-c9346f15-aeb1-4afc-a181-64cb583e7ffc.png)
![akhq-queries](https://user-images.githubusercontent.com/1021324/210272528-906db4f5-5ca6-42f5-aabb-6dc557a2fba8.png)
The server info
![akhq-server-info](https://user-images.githubusercontent.com/1021324/210272542-ffc8d8f1-0beb-47ba-b3c3-600f7742505d.png)
The DDL execution window activated with the button "Execute statement"
![akhq-ddl](https://user-images.githubusercontent.com/1021324/210272597-8f168c54-c2aa-4084-8a43-a44ad9064737.png)
And the DML window activated with the button "Execute query"
![akhq-dml](https://user-images.githubusercontent.com/1021324/210272649-5d3e4924-87bf-43dc-8a4c-0b91d548b79d.png)
![akhq-dml-result](https://user-images.githubusercontent.com/1021324/210272655-57b2df89-6f4f-48e9-bbe0-317c7fb09474.png)
Looking forward to hear your feedback.