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

fix(ui): add null check to find overlapping blocks logic #7644

Merged
merged 2 commits into from
Aug 27, 2024

Conversation

pureiboi
Copy link
Contributor

@pureiboi pureiboi commented Aug 15, 2024

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

Changes

Add null check to find overlapping blocks logic to solve #5359.

Verification

tested locally, compare result with both unchecked vs checked

thanos_uncheck thanos_checked

@pureiboi pureiboi force-pushed the fix/block-overlapping-ui branch 2 times, most recently from 8c5da82 to 4eb31aa Compare August 15, 2024 16:41
@pureiboi
Copy link
Contributor Author

e2e GO test 8,5 and 8,7 were facing connection refused doubt this is related to the changes i committed

react / React UI test on Node 14 test, didn't seems to be related to commit.

any advice on this?

@@ -156,7 +156,11 @@ export const getFilteredBlockPools = (
const poolArrayIndex = blockPools[key];
const poolArray = poolArrayIndex[Object.keys(poolArrayIndex)[0]];
for (let i = 0; i < filteredBlocks.length; i++) {
if (JSON.stringify(filteredBlocks[i].thanos.labels) === JSON.stringify(poolArray[0][0].thanos.labels)) {
if (
Copy link
Member

Choose a reason for hiding this comment

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

Would it be possible to add a unit test for this? We typically add tests together with these fixes to avoid accidental regressions in the future.

Copy link
Contributor Author

@pureiboi pureiboi Aug 16, 2024

Choose a reason for hiding this comment

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

the newly added test data should cover the unit test for regression

If I were to remove the if condition, the error would flag up.

image

having the condition implemented, unit is able to pass

image

Signed-off-by: pureiboi <17396188+pureiboi@users.noreply.github.com>
@pureiboi pureiboi force-pushed the fix/block-overlapping-ui branch from acf18d4 to eb9469f Compare August 20, 2024 02:19
@pureiboi
Copy link
Contributor Author

fixed failure on react / React UI test

Copy link
Member

@GiedriusS GiedriusS left a comment

Choose a reason for hiding this comment

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

Cannot reproduce locally, good work 🥇

@GiedriusS GiedriusS merged commit ce52e9f into thanos-io:main Aug 27, 2024
22 checks passed
jnyi pushed a commit to jnyi/thanos that referenced this pull request Oct 17, 2024
…7644)

Signed-off-by: pureiboi <17396188+pureiboi@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants