Skip to content

⚠️ allow filtering with a list of channels - #1173

Merged
joelanford merged 1 commit into
operator-framework:mainfrom
everettraven:feature/channels-filtering
Sep 5, 2024
Merged

⚠️ allow filtering with a list of channels#1173
joelanford merged 1 commit into
operator-framework:mainfrom
everettraven:feature/channels-filtering

Conversation

@everettraven

@everettraveneverettraven commented Aug 26, 2024

Copy link
Copy Markdown
Contributor

Description

Reviewer Checklist

  • API Go Documentation
  • Tests: Unit Tests (and E2E Tests, if appropriate)
  • Comprehensive Commit Messages
  • Links to related GitHub Issue(s)

@everettraven
everettraven requested a review from a team as a code ownerAugust 26, 2024 16:14
@netlify

netlifyBot commented Aug 26, 2024

Copy link
Copy Markdown

Deploy Preview for olmv1 ready!

NameLink
🔨 Latest commit690c2ce
🔍 Latest deploy loghttps://app.netlify.com/sites/olmv1/deploys/66d9f873f5f8e6000812f7fa
😎 Deploy Previewhttps://deploy-preview-1173--olmv1.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@codecov

codecovBot commented Aug 26, 2024

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 92.30769% with 4 lines in your changes missing coverage. Please review.

Project coverage is 76.49%. Comparing base (75bb03d) to head (690c2ce).
Report is 3 commits behind head on main.

Files with missing linesPatch %Lines
api/v1alpha1/zz_generated.deepcopy.go0.00%3 Missing and 1 partial ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #1173 +/- ##
==========================================
- Coverage 76.59% 76.49% -0.11% 
==========================================
Files 40 40 Lines 2329 2340 +11 ==========================================
+ Hits 1784 1790 +6 - Misses 389 393 +4 - Partials 156 157 +1 
FlagCoverage Δ
e2e57.64% <55.76%> (-0.06%)⬇️
unit52.43% <92.30%> (+<0.01%)⬆️

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

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

Comment threadinternal/controllers/clusterextension_controller.go Outdated
Comment threadinternal/resolve/catalog.go Outdated
ResolvedBundles []*declcfg.Bundle
}

func buildResolutionError(rei resolutionErrorInfo) error {

@tmshorttmshortAug 27, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I tried this once (an error generation function) and there was opposition to it. At that time, it was called in multiple places, this seems to be simply pulling out the code into a function, without any additional callers.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

(And of course, this requires changing all the error messages)

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I pulled it out into a separate function because the linter was complaining about the nesting depth being too high.

Regarding changing all the error messages, I found it a bit more complex to read the previous error logic in a switch statement and thought it was a bit more readable using this new strings.Builder approach because it is more linear in nature (i.e as you read each line of the code you see the scenarios that result in new information being added to the error string we return).

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

If folks prefer the previous approach, I can revert this change.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't care, I just wanted to make the note.

Comment threadinternal/resolve/catalog_test.go Outdated
tmshort
tmshort previously approved these changes Aug 28, 2024

@tmshorttmshort left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

/lgtm

@openshift-ciopenshift-ciBot added the lgtm Indicates that a PR is ready to be merged. label Aug 28, 2024
@openshift-merge-robotopenshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 30, 2024
@openshift-ciopenshift-ciBot removed the lgtm Indicates that a PR is ready to be merged. label Sep 3, 2024
@openshift-ci

Copy link
Copy Markdown

New changes are detected. LGTM label has been removed.

@everettraven
everettravenforce-pushed the feature/channels-filtering branch from 5c0112d to c720e73CompareSeptember 3, 2024 18:25
@openshift-merge-robotopenshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 3, 2024
Comment threadinternal/controllers/clusterextension_controller.go Outdated
Comment threadinternal/controllers/clusterextension_controller_test.go
Comment threadinternal/resolve/catalog.go
Comment threadinternal/resolve/catalog.go Outdated
Comment threadinternal/resolve/catalog.go Outdated
Comment threadinternal/resolve/catalog.go Outdated
Comment threadinternal/resolve/catalog_test.go Outdated
Comment threadinternal/resolve/catalog_test.go
@everettraven
everettravenforce-pushed the feature/channels-filtering branch from c720e73 to dc90ec5CompareSeptember 4, 2024 21:17
@openshift-merge-robotopenshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 4, 2024
@everettraven
everettravenforce-pushed the feature/channels-filtering branch from dc90ec5 to 7435dacCompareSeptember 4, 2024 21:18
@openshift-merge-robotopenshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 4, 2024
Type: ocv1alpha1.TypeChannelDeprecated,
Reason: ocv1alpha1.ReasonDeprecated,
Status: metav1.ConditionTrue,
ObservedGeneration: 1,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hmm, I didn't realize this test ignored the message. Follow-up?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Is there a specific reason we should be testing the message?

I understand wanting to verify our message crafting logic is correct, but I lean towards the camp where the message is any arbitrary string and is not part of our API guarantees - testing for it explicitly makes our unit tests brittle

Comment threadinternal/resolve/catalog.go Outdated
Comment threadinternal/resolve/catalog.go Outdated
@everettraven
everettravenforce-pushed the feature/channels-filtering branch from 29458cc to 8369bf5CompareSeptember 5, 2024 18:24
Signed-off-by: everettraven <everettraven@gmail.com>
@everettraven
everettravenforce-pushed the feature/channels-filtering branch from 173064d to 690c2ceCompareSeptember 5, 2024 18:29
@joelanford
joelanford added this pull request to the merge queue Sep 5, 2024
Merged via the queue into operator-framework:main with commit c4470ccSep 5, 2024
@skattojuskattoju mentioned this pull request Sep 25, 2024
4 tasks
Sign up for freeto 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.

Update the ClusterExtension.Spec.Channel field to be a list of channels

4 participants

@everettraven@joelanford@tmshort@openshift-merge-robot