🌱 Upgrade Deppy #560

m1kola · 2023-12-01T11:37:04Z

Description

New version contains breaking changes: Deppy no longer has variable source API. Instead it accepts a slice of variables as input.

Reviewer Checklist

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

codecov · 2023-12-01T11:43:04Z

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (6cb7029) 83.93% compared to head (5646c12) 84.06%.
Report is 1 commits behind head on main.

Files	Patch %	Lines
internal/controllers/operator_controller.go	71.42%	3 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #560      +/-   ##
==========================================
+ Coverage   83.93%   84.06%   +0.13%     
==========================================
  Files          20       20              
  Lines         809      816       +7     
==========================================
+ Hits          679      686       +7     
  Misses         90       90              
  Partials       40       40

Flag	Coverage Δ
e2e	`63.35% <77.77%> (-0.43%)`	⬇️
unit	`78.97% <73.91%> (+0.22%)`	⬆️

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.

everettraven

Overall looks good to me. One nit about potentially confusing naming of a field but I don't think that should block this PR

everettraven · 2023-12-01T14:57:33Z

internal/controllers/operator_controller.go

 // OperatorReconciler reconciles a Operator object
 type OperatorReconciler struct {
 	client.Client
-	Scheme   *runtime.Scheme
-	Resolver *solver.DeppySolver
+	CatalogClient BundleProvider


Nit: I can see confusion occurring with this naming pattern. If unaware of the context one might assume that this is a fully featured client for retrieving catalog information where in reality this only provides an interface for fetching bundles. I'd prefer a name that more clearly signals that this is used for fetching bundles specifically. Maybe something like BundleClient?

I renamed the field to be BundleProvider to match BundleProvider interface. But I kept catalog client names in the client itself (at least for now). The client already fetches all the data from catalogs - It is just that we only provide a method to get bundles. This might change at some point.

tmshort

The use of the name variables in multiple places might be a bit confusing... I'd suggest using vars in places where there's a function/method named variables. Or possibly renaming the variables() method. (Yes, golang is able to figure it out, but can we?)

internal/controllers/operator_controller.go

ncdc · 2023-12-04T14:59:24Z

I was about to approve (I'm ok w/the naming) but want to give @m1kola a chance to respond

ncdc · 2023-12-04T15:35:01Z

Re ⚠️ - while deppy had breaking changes, users won't experience any breaking changes, will they?

m1kola · 2023-12-04T15:37:07Z

Re ⚠️ - while deppy had breaking changes, users won't experience any breaking changes, will they?

Good point. I think for operator-controller users everything should stay the same. Replaced with 🌱

New version contains breaking changes: Deppy no longer has variable source API. Instead it accepts a slice of variables as input. Signed-off-by: Mikalai Radchuk <mradchuk@redhat.com>

tmshort

/lgtm

m1kola · 2023-12-04T16:43:29Z

Quay failed with 502. Will try to merge again.

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 1, 2023

m1kola force-pushed the update_deppy branch 2 times, most recently from 5cdfb1b to 5554ac6 Compare December 1, 2023 11:52

m1kola marked this pull request as ready for review December 1, 2023 12:03

m1kola requested a review from a team as a code owner December 1, 2023 12:03

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 1, 2023

everettraven previously approved these changes Dec 1, 2023

View reviewed changes

m1kola dismissed everettraven’s stale review via 5646c12 December 1, 2023 16:58

m1kola force-pushed the update_deppy branch from 5554ac6 to 5646c12 Compare December 1, 2023 16:58

m1kola requested a review from everettraven December 1, 2023 17:09

m1kola enabled auto-merge December 4, 2023 11:46

tmshort requested changes Dec 4, 2023

View reviewed changes

internal/controllers/operator_controller.go Outdated Show resolved Hide resolved

internal/controllers/operator_controller.go Outdated Show resolved Hide resolved

m1kola changed the title ~~⚠️ Upgrade Deppy~~ 🌱 Upgrade Deppy Dec 4, 2023

Upgrade Deppy

44e2381

New version contains breaking changes: Deppy no longer has variable source API. Instead it accepts a slice of variables as input. Signed-off-by: Mikalai Radchuk <mradchuk@redhat.com>

m1kola force-pushed the update_deppy branch from 5646c12 to 44e2381 Compare December 4, 2023 15:47

ncdc approved these changes Dec 4, 2023

View reviewed changes

m1kola requested a review from tmshort December 4, 2023 15:54

m1kola mentioned this pull request Dec 4, 2023

🌱 Bump github.com/operator-framework/operator-registry from 1.32.0 to 1.33.0 #557

Merged

tmshort approved these changes Dec 4, 2023

View reviewed changes

m1kola added this pull request to the merge queue Dec 4, 2023

openshift-ci bot assigned tmshort Dec 4, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 4, 2023

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 4, 2023

m1kola added this pull request to the merge queue Dec 4, 2023

Merged via the queue into operator-framework:main with commit ba21cdf Dec 4, 2023
13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🌱 Upgrade Deppy #560

🌱 Upgrade Deppy #560

m1kola commented Dec 1, 2023

codecov bot commented Dec 1, 2023 •

edited

Loading

everettraven left a comment

everettraven Dec 1, 2023

m1kola Dec 1, 2023

tmshort left a comment

ncdc commented Dec 4, 2023

ncdc commented Dec 4, 2023

m1kola commented Dec 4, 2023 •

edited

Loading

tmshort left a comment

m1kola commented Dec 4, 2023

🌱 Upgrade Deppy #560

🌱 Upgrade Deppy #560

Conversation

m1kola commented Dec 1, 2023

Description

Reviewer Checklist

codecov bot commented Dec 1, 2023 • edited Loading

Codecov Report

everettraven left a comment

Choose a reason for hiding this comment

everettraven Dec 1, 2023

Choose a reason for hiding this comment

m1kola Dec 1, 2023

Choose a reason for hiding this comment

tmshort left a comment

Choose a reason for hiding this comment

ncdc commented Dec 4, 2023

ncdc commented Dec 4, 2023

m1kola commented Dec 4, 2023 • edited Loading

tmshort left a comment

Choose a reason for hiding this comment

m1kola commented Dec 4, 2023

codecov bot commented Dec 1, 2023 •

edited

Loading

m1kola commented Dec 4, 2023 •

edited

Loading