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

block (UI and API): Add mark deletion and no compaction UI #4620

Merged
merged 12 commits into from
Nov 6, 2021

Conversation

adzshaf
Copy link
Contributor

@adzshaf adzshaf commented Sep 1, 2021

This is part of LFX mentorship project (issue: #3112). We want to add mark deletion and no compaction button in block details UI.

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

  • Add /block/mark endpoint for trigger marking block API
  • Add button for mark deletion and no compaction in UI, with modal for asking the detail/reason

Verification

Tested in local using make quickstart

Example of mark deletion:
Example Deletion

Result of mark deletion:
image

Example of mark no compaction:
Example No Compaction

Result of mark no compaction:
image

cc: @squat @onprem

bwplotka
bwplotka previously approved these changes Sep 1, 2021
Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

It looks pretty sweet, thank your!

I would be happy to merge it TBH, LGTM!

}

switch actionParam {
case "DELETION":
Copy link
Member

Choose a reason for hiding this comment

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

Can move this to Go content so we don't make typo/mistake here?

@bwplotka
Copy link
Member

bwplotka commented Sep 1, 2021

Do you mind rebasing to fix bindata?

@yeya24
Copy link
Contributor

yeya24 commented Sep 2, 2021

What about having adding this to the latest release? @bwplotka

Copy link
Contributor

@yeya24 yeya24 left a comment

Choose a reason for hiding this comment

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

Let's fix the comment, resolve the conflicts and add a changelog entry? Then we are ready to merge it I think.

@yeya24
Copy link
Contributor

yeya24 commented Oct 30, 2021

Just added a commit to resolve conflict. I feel this is ready to go! cc @onprem @adzshaf

onprem
onprem previously approved these changes Nov 2, 2021
pkg/ui/react-app/src/thanos/pages/blocks/BlockDetails.tsx Outdated Show resolved Hide resolved
pkg/api/blocks/v1.go Outdated Show resolved Hide resolved
adzshaf and others added 12 commits November 6, 2021 11:17
Signed-off-by: Shafiya Adzhani <[email protected]>
Signed-off-by: Shafiya Adzhani <[email protected]>
Signed-off-by: Shafiya Adzhani <[email protected]>
Signed-off-by: Shafiya Adzhani <[email protected]>
Signed-off-by: Shafiya Adzhani <[email protected]>
Signed-off-by: Shafiya Adzhani <[email protected]>
Update pkg/api/blocks/v1.go

Co-authored-by: Prem Kumar <[email protected]>

Signed-off-by: Ben Ye <[email protected]>
@yeya24
Copy link
Contributor

yeya24 commented Nov 6, 2021

Sorry for my poor git skill. I rebased the code and all previous commits are also signed off by me, too. Hope this is okay @adzshaf.

I fixed the bindata and tested it working locally. Overall this looks good, but there is some work to fix:

In the dark mode, I expect the button to have a lighter color.
image

Also after I click Mark Deletion or Mark No Compaction, it works and I can see that the marker created. But in the UI I didn't get any status response to tell me whether the work succeeds or not. This is something important to have.

@yeya24 yeya24 enabled auto-merge (squash) November 6, 2021 18:46
Copy link
Contributor

@yeya24 yeya24 left a comment

Choose a reason for hiding this comment

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

LGTM. Let's fix nits in next pr.

@yeya24 yeya24 merged commit 65c5bc5 into thanos-io:main Nov 6, 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.

4 participants