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

Respect delete markers in package-related ES queries #2000

Merged
merged 7 commits into from
Jan 17, 2021

Conversation

nl0
Copy link
Member

@nl0 nl0 commented Jan 13, 2021

TODO

  • handle delete markers for packages in search results

@nl0
Copy link
Member Author

nl0 commented Jan 13, 2021

2000 GET BTW

@codecov-io
Copy link

codecov-io commented Jan 14, 2021

Codecov Report

Merging #2000 (c8efca3) into master (dcd4d3a) will increase coverage by 0.01%.
The diff coverage is 12.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2000      +/-   ##
==========================================
+ Coverage   46.71%   46.72%   +0.01%     
==========================================
  Files         332      332              
  Lines       16450    16447       -3     
  Branches     2195     2196       +1     
==========================================
+ Hits         7684     7685       +1     
+ Misses       7867     7863       -4     
  Partials      899      899              
Flag Coverage Δ
api-python 89.35% <ø> (ø)
catalog 7.39% <0.00%> (+<0.01%) ⬆️
lambda 92.40% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
catalog/app/containers/Bucket/Overview.js 0.00% <0.00%> (ø)
catalog/app/containers/Bucket/requests.js 0.00% <0.00%> (ø)
lambdas/search/tests/test_search.py 100.00% <ø> (ø)
lambdas/search/index.py 88.63% <100.00%> (+0.13%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dcd4d3a...c8efca3. Read the comment docs.

@fiskus
Copy link
Member

fiskus commented Jan 14, 2021

If it's ready for review, could you provide an example of how can I test it manually?

@nl0
Copy link
Member Author

nl0 commented Jan 14, 2021

@fiskus

If it's ready for review, could you provide an example of how can I test it manually?

1. Package counts

Package counts in bucket overview should now show the total number of revisions, so you can try revising a package and that should increment that counter. You could also try deleting the revision and see the counter decrement (tho i'm not sure how to do that -- you have to use Quilt CLI or Python API)

2. Package and revision listing

Only non-deleted packages should be shown in the package list with correct revision counts (in general, package listing should correspond to the .quilt/named_packages dir contents).

The same with the revisions: revision list (both in breadcrumbs dropdown and on the separate page) should not include deleted revisions (the same here: revision listing should correspond to the contents of .quilt/named_packages/$handle).

Again, to properly test this you should create and then delete packages / revisions via Quilt Python API / CLI.

Also, currently there's a bug Aneesh was talking about today that prevents inserting all the delete markers into ES index, so it's kinda broken at the moment.

@fiskus
Copy link
Member

fiskus commented Jan 14, 2021

  1. I see a package number increment on revising a package with searchminimal stack. On staging stack API has 500 error. For me, it's counter-intuitive, that now it's still "123 packages" but actually "123 package versions".

@fiskus
Copy link
Member

fiskus commented Jan 14, 2021

  1. b/aneesh-ai2-temp/packages/ on staging stack shows "No packages", on searchminimal stack shows deleted package akarve/ds121 (don't remember exact name). As I see, it's not correct in both cases.

@akarve akarve merged commit 06b2387 into master Jan 17, 2021
@akarve akarve deleted the es-delete-markers-pkg branch January 17, 2021 21:35
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