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 feature: Empty topic button #258

Merged
merged 22 commits into from Sep 14, 2020

Conversation

clallavena
Copy link
Contributor

Add feature of delete all records from a selected topic in one button

@clallavena clallavena marked this pull request as ready for review April 3, 2020 16:41
@clallavena clallavena changed the title Enhancement/empty topic Add feature: Empty topic button Apr 5, 2020
@clallavena
Copy link
Contributor Author

Idk why the check failed at slack notification 🤔

@tchiotludo
Copy link
Owner

I must modify the github actions, the secret for the slack notification are not available for a PR (normal for a secret) so I must disabled some check on github action.
(move to github actions are recent and not well tested for PR)

@clallavena
Copy link
Contributor Author

I must modify the github actions, the secret for the slack notification are not available for a PR (normal for a secret) so I must disabled some check on github action.
(move to github actions are recent and not well tested for PR)

Ho ok that make sense

@tchiotludo
Copy link
Owner

@clallavena : I've merged on dev a lot of changed, now there is an api for AKHQ.
You will need to rebase your work from dev since I made a lot of change.

Also you will have to add an API endpoint for this new feature (now all feature must have an api endpoint also).

I've a quick look on the PR, seems to be a good feature and there will be only minor changed I think.
Maybe a nice addition will be allow to drop data till a timestamp (and not the whole topic) and will be a most common use case I think, since Kafka api allow to fetch offset by timestamp, this only need a intermediate page like consumer group offset changed that allow user to choose specific offset, timestamp offset or drop all (first offset).

Tell me what do you think about that.

@clallavena
Copy link
Contributor Author

clallavena commented Apr 7, 2020

@clallavena : I've merged on dev a lot of changed, now there is an api for AKHQ.
You will need to rebase your work from dev since I made a lot of change.

I've a quick look on the PR, seems to be a good feature and there will be only minor changed I think.
Maybe a nice addition will be allow to drop data till a timestamp (and not the whole topic) and will be a most common use case I think, since Kafka api allow to fetch offset by timestamp, this only need a intermediate page like consumer group offset changed that allow user to choose specific offset, timestamp offset or drop all (first offset).

Tell me what do you think about that.

Yeah I like the idea ! It's a good add-on to the empty topic feature. I'm currently trying to adapt the current version of the feature with the new API, i'm a little bit stuck with API unit tests that throw me assertion error with listApi(), but i'm on it.

Also you will have to add an API endpoint for this new feature (now all feature must have an api endpoint also).

I've just add the endpoint for the emtpyTopic in the API, but it's currently in my local change for now.

@clallavena
Copy link
Contributor Author

@tchiotludo I've been thinking about the improvement of this new feature, and I think it's a good idea however I think we should let the "empty topic" because it can be anoying to get to the delete page for empty a topic, especially if it's an "everyday action".

So i'm totaly on a new page for the deletion but i'm also with letting the "empty button" in place.

Are you ok with that ?

@tchiotludo
Copy link
Owner

Yes maybe on the confirmation popup you can add a button the go to the page with more details ?

@clallavena
Copy link
Contributor Author

Yes maybe on the confirmation popup you can add a button the go to the page with more details ?

Yes it may be a good option !

@FrankMormino
Copy link
Contributor

hey @clallavena and @tchiotludo - appreciate this work guys! Awesome job here! Any timeline for when this gets into the dev branch?

@clallavena
Copy link
Contributor Author

Hey @FrankMormino, ty for that. Yeah I just made the empty topic button for now and I did not have the time this past few weeks to develop the idea of @tchiotludo. But soon I hope to develop the option soon and made it available for the next update :)

@FrankMormino
Copy link
Contributor

Awesome thanks!

@FrankMormino
Copy link
Contributor

hey @clallavena if you'd like i could test a pre-release branch or something to help validate this works as expected when you have time? thanks

@FrankMormino
Copy link
Contributor

Excited for this guy!

@FrankMormino
Copy link
Contributor

looks like the last commit was on deleted branch.
This commit does not belong to any branch on this repository.

@clallavena
Copy link
Contributor Author

looks like the last commit was on deleted branch.
This commit does not belong to any branch on this repository.

@FrankMormino this branch belong to my fork of akhq so it seems normal to doesnt find this branch on the origin repo :)

I'm working on the ask enhancement, the test part is difficult for my computer (it's not the powerfull one) so it may be slower than you can expect but i'm on it as soon I've time available for it :)

@tchiotludo
Copy link
Owner

@clallavena : the new ui is here !
I have more time to deal with this PR.

Do you want to continue this PR for the new UI ?
In my opinion, I will merge without all the modification I asked (offsets, etc ...). Just the empty button is fined !
This can be deal on another PR I think.

If you are ok, just add the button on the topiclist and remove all the old ui (that will be drop) and this PR will be merged for next release

What do you think about that ?

@clallavena
Copy link
Contributor Author

@tchiotludo It's fine for me! I was working on the modification you were asking and the process part was good only the UI part was missing and was difficult for me ( due to my knowledge in FreeMarker ), I will rebase my previous work and I will just add the empty topic button in my next commit so you can review it.

I'm currently not able to finish that until the next week but I will fix this quick.

Can't wait to see the new UI ! I will in my return.

@FrankMormino
Copy link
Contributor

hey i just pulled latest snapshot for dev - where will this option be shown - checked the topic list and not seeing it but could be somewhere very obvious that I am just missing. Thanks again for this feature!

@tchiotludo
Copy link
Owner

The features is not merged, so not available on the dev branch.
For now, there isn't any button on the PR !

@clallavena
Copy link
Contributor Author

Hello, no it's normal I've to learn a little about React, i've never use it before so i'm digging into it in way to add the button in my next commit :) don't worry it will be soon

@clallavena
Copy link
Contributor Author

Hey @tchiotludo :)

I've finally done with that feature. Only things that make my mind in trouble Is that when the empty topic is confirm the page keep charging on nothing, you've to refresh the page to return on the topic data page. And idk why.

A little help on that to help me understand my mistake is welcome. :)

@tchiotludo tchiotludo merged commit c42b6e9 into tchiotludo:dev Sep 14, 2020
Backlog automation moved this from Review in progress to Done Sep 14, 2020
tchiotludo added a commit that referenced this pull request Sep 14, 2020
@tchiotludo
Copy link
Owner

Hey @clallavena
Thanks for the work almost ready !!
I just add a little commit to make it works perfectly !

Thanks for the work and sorry for huge delay on this ! 👍

@tchiotludo
Copy link
Owner

seeing your email, maybe you can contribute here also : #161 😄

@FrankMormino
Copy link
Contributor

hey when testing this - seeing this
image
Do you want me to create new thread to discuss this question?

@FrankMormino
Copy link
Contributor

hey when testing this - seeing this
image
Do you want me to create new thread to discuss this question?

added issue reference
#399

@clallavena
Copy link
Contributor Author

Thank you @tchiotludo for fix my mistake, and no problem about the delay ! 😄

Yep I would like to do so for the #161, I'm currently working on that as well in my organisation and i hope we can figure on this 😉

I'll be back on you very soon on this.

yanayita pushed a commit to yanayita/akhq that referenced this pull request Oct 1, 2020
Co-authored-by: Clement Allavena <FP17690@adgroup.michelin.com>
yanayita pushed a commit to yanayita/akhq that referenced this pull request Oct 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Backlog
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants