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

Index S3 tags #3691

Merged
merged 8 commits into from
Aug 22, 2023
Merged

Index S3 tags #3691

merged 8 commits into from
Aug 22, 2023

Conversation

sir-sigurd
Copy link
Member

@sir-sigurd sir-sigurd commented Aug 14, 2023

Description

This indexes S3 object tags as text i.e. like this "key1 value1 key2 value2 ..." into s3_tags field.

TODO

  • Unit tests
  • Changelog entry (skip if change is not significant to end users, e.g. docs only)

@codecov
Copy link

codecov bot commented Aug 14, 2023

Codecov Report

Merging #3691 (5efe3a9) into master (8166124) will decrease coverage by 7.87%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #3691      +/-   ##
==========================================
- Coverage   36.12%   28.26%   -7.87%     
==========================================
  Files         677      632      -45     
  Lines       29677    25643    -4034     
  Branches     4327     4327              
==========================================
- Hits        10720     7247    -3473     
+ Misses      17825    17264     -561     
  Partials     1132     1132              
Flag Coverage Δ
api-python 91.32% <ø> (-0.04%) ⬇️
catalog 9.83% <ø> (ø)
lambda ?

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

see 47 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@sir-sigurd sir-sigurd marked this pull request as ready for review August 15, 2023 10:48
@sir-sigurd sir-sigurd requested a review from akarve August 15, 2023 10:54
akarve
akarve previously approved these changes Aug 17, 2023
@akarve
Copy link
Member

akarve commented Aug 17, 2023

Please add a comment and TODO (maybe in the doc string for the function) that we currently do not handle object tagging events therefore, contrary to design (but as agreed per my suggestion) S3 can drift from ES since tag changes do not incur object events that we rely on today.

Copy link
Member

@nl0 nl0 left a comment

Choose a reason for hiding this comment

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

lgtm, tho indexing tags as key1 value1 key2 value2 ... feels like a questionable decision tbh

@sir-sigurd
Copy link
Member Author

lgtm, tho indexing tags as key1 value1 key2 value2 ... feels like a questionable decision tbh

yes, though it was promoted by @akarve

@nl0
Copy link
Member

nl0 commented Aug 22, 2023

lgtm, tho indexing tags as key1 value1 key2 value2 ... feels like a questionable decision tbh

yes, though it was promoted by @akarve

i don't urge anyone to re-evaluate the solution atm, but we may consider revisiting that some time in the future

@sir-sigurd sir-sigurd added this pull request to the merge queue Aug 22, 2023
Merged via the queue into master with commit 683809b Aug 22, 2023
40 checks passed
@sir-sigurd sir-sigurd deleted the index-s3-tags branch August 22, 2023 10:15
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