Skip to content

pkg/storage: handle more possible release label k/v pairs #399

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

Conversation

joelanford
Copy link
Member

@joelanford joelanford commented Oct 17, 2024

Release labels are currently stored only in Kubernetes object metadata, which limits what can actually be stored there.

In order to support a wider variety of release labels, we will only store system labels in the index secret. All custom release labels will be serialized in the release blob itself.

Release labels are stored only in Kubernetes object metadata. In order
to support a wider variety of release labels, check release label
validity. If a release label is invalid for a kubernetes metadata.label
store it as an annotation instead.

Signed-off-by: Joe Lanford <joe.lanford@gmail.com>
Copy link

@tmshort tmshort left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 17, 2024
@codecov-commenter
Copy link

codecov-commenter commented Oct 17, 2024

Codecov Report

Attention: Patch coverage is 91.17647% with 3 lines in your changes missing coverage. Please review.

Project coverage is 78.48%. Comparing base (08ab7fb) to head (cceec2e).
Report is 52 commits behind head on main.

Files with missing lines Patch % Lines
pkg/storage/chunked.go 91.17% 1 Missing and 2 partials ⚠️

❗ There is a different number of reports uploaded between BASE (08ab7fb) and HEAD (cceec2e). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (08ab7fb) HEAD (cceec2e)
2 1
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #399      +/-   ##
==========================================
- Coverage   85.06%   78.48%   -6.59%     
==========================================
  Files          19       31      +12     
  Lines        1346     2477    +1131     
==========================================
+ Hits         1145     1944     +799     
- Misses        125      444     +319     
- Partials       76       89      +13     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 17, 2024
Copy link

openshift-ci bot commented Oct 17, 2024

New changes are detected. LGTM label has been removed.

Comment on lines +333 to +336
// The only labels that get stored on the index secret are system labels, so we'll do a two-pass
// query. First, we'll request index secrets from the API server that match the query labels that
// are system labels. From there, we decode the releases that match, and then further filter those
// based on the rest of the query labels that are not system labels.
Copy link
Member Author

Choose a reason for hiding this comment

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

This was necessary to get existing tests to pass. We have a test that uses a custom label in a query.

@joelanford joelanford force-pushed the promote-invalid-labels-to-annotation branch from 69971bb to 787247e Compare October 17, 2024 20:28
Signed-off-by: Joe Lanford <joe.lanford@gmail.com>
@joelanford joelanford force-pushed the promote-invalid-labels-to-annotation branch from 787247e to cceec2e Compare October 17, 2024 20:29
@joelanford joelanford added this pull request to the merge queue Oct 17, 2024
Merged via the queue into operator-framework:main with commit a3b49f3 Oct 17, 2024
6 checks passed
@joelanford joelanford deleted the promote-invalid-labels-to-annotation branch October 17, 2024 20:50
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.

4 participants