🌱 Decompose RegistryV1ToHelmChart function #1735

perdasilva · 2025-02-10T12:04:01Z

Description

Refactor RegistryV1ToHelmChart function in the rukpak/convert package by taking out the code that parses a bundle filesystem into a RegistryV1 object and putting it into its own function ParseFS. This facilitates manipulation of bundles in other activities, e.g. tooling/CLI

Reviewer Checklist

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

netlify · 2025-02-10T12:04:18Z

✅ Deploy Preview for olmv1 ready!

Name	Link
🔨 Latest commit	`6cf4dbe`
🔍 Latest deploy log	https://app.netlify.com/sites/olmv1/deploys/67acb66b5183a60008a25f46
😎 Deploy Preview	https://deploy-preview-1735--olmv1.netlify.app
📱 Preview on mobile	Toggle 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 · 2025-02-10T12:17:34Z

Codecov Report

Attention: Patch coverage is 59.37500% with 13 lines in your changes missing coverage. Please review.

Project coverage is 67.45%. Comparing base (c9fb138) to head (6cf4dbe).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...l/operator-controller/rukpak/convert/registryv1.go	59.37%	9 Missing and 4 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1735      +/-   ##
==========================================
+ Coverage   67.34%   67.45%   +0.11%     
==========================================
  Files          61       61              
  Lines        5236     5245       +9     
==========================================
+ Hits         3526     3538      +12     
+ Misses       1449     1446       -3     
  Partials      261      261

Flag	Coverage Δ
e2e	`52.15% <50.00%> (-0.05%)`	⬇️
unit	`55.00% <37.50%> (+<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.

tmshort · 2025-02-10T16:42:22Z

broken unit test due to removal of function.

camilamacedo86

Some lint issues otherwise it seems LGTM to me 👍

perdasilva · 2025-02-11T07:23:27Z

broken unit test due to removal of function.

Sorry about that. Shot from the hip as I was getting rushed off to dinner. I've fixed it up by removing the unit test. The test below it performs the same test. I also moved it to the test package to help us avoid testing private functions in the future.

perdasilva · 2025-02-11T08:57:58Z

internal/rukpak/convert/registryv1_test.go

 	require.Nil(t, plainBundle)
 }

-func TestRegistryV1SuiteGeneratePropagateCsvAnnotations(t *testing.T) {


Removed TestRegistryV1SuiteGeneratePropagateCsvAnnotations since it was testing an unexported function and the behavior is also tested on line 489

I think I got your change 👍

camilamacedo86 · 2025-02-11T13:34:18Z

internal/rukpak/convert/registryv1_test.go

+spec:
+  installModes:
+    - type: AllNamespaces
+      supported: true


I can see here that we still using the same mock from what was removed csv.Spec.InstallModes = []v1alpha1.InstallMode{{Type: v1alpha1.InstallModeTypeMultiNamespace, Supported: true}}

camilamacedo86 · 2025-02-11T13:36:00Z

internal/rukpak/convert/registryv1.go

+			Name: fmt.Sprintf("object-%x.json", hash[0:8]),
+			Data: jsonData,
+		})
+	}


👍 I can see that we are doing mainly the same just/refac/cleanup

camilamacedo86

/lgtm

LalatenduMohanty · 2025-02-11T18:34:12Z

@perdasilva #1737 got merged, so this PR need a rebase.

Signed-off-by: Per Goncalves da Silva <pegoncal@redhat.com>

Signed-off-by: Per Goncalves da Silva <pegoncal@redhat.com> Signed-off-by: Per G. da Silva <pegoncal@redhat.com>

camilamacedo86

/lgtm

When pass in the CI ( flake )

openshift-ci · 2025-02-12T14:55:40Z

New changes are detected. LGTM label has been removed.

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 Feb 10, 2025

perdasilva marked this pull request as ready for review February 10, 2025 15:51

perdasilva requested a review from a team as a code owner February 10, 2025 15:51

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 Feb 10, 2025

perdasilva force-pushed the refactor-render branch from e4070ec to 26add31 Compare February 10, 2025 16:06

perdasilva changed the title ~~🌱 Factor FS to RegistryV1 code out of helm conversion~~ 🌱 Decompose RegistryV1ToHelmChart function Feb 10, 2025

camilamacedo86 reviewed Feb 10, 2025

View reviewed changes

perdasilva force-pushed the refactor-render branch from 26add31 to e8ce6c8 Compare February 11, 2025 07:22

perdasilva force-pushed the refactor-render branch 2 times, most recently from 0fb3c14 to 0bc6f80 Compare February 11, 2025 08:56

perdasilva commented Feb 11, 2025

View reviewed changes

perdasilva force-pushed the refactor-render branch 2 times, most recently from 042a261 to e36a985 Compare February 11, 2025 12:48

camilamacedo86 reviewed Feb 11, 2025

View reviewed changes

camilamacedo86 previously approved these changes Feb 11, 2025

View reviewed changes

openshift-ci bot assigned camilamacedo86 Feb 11, 2025

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 11, 2025

perdasilva added this pull request to the merge queue Feb 11, 2025

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 11, 2025

perdasilva added this pull request to the merge queue Feb 11, 2025

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 11, 2025

perdasilva force-pushed the refactor-render branch from e36a985 to da80fa2 Compare February 11, 2025 15:25

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Feb 11, 2025

perdasilva force-pushed the refactor-render branch from da80fa2 to 985c5db Compare February 11, 2025 16:12

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 11, 2025

perdasilva dismissed camilamacedo86’s stale review via c9b3130 February 12, 2025 09:08

perdasilva force-pushed the refactor-render branch from 985c5db to c9b3130 Compare February 12, 2025 09:08

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 12, 2025

joelanford previously approved these changes Feb 12, 2025

View reviewed changes

perdasilva dismissed joelanford’s stale review via 5bed60c February 12, 2025 14:05

perdasilva force-pushed the refactor-render branch from c9b3130 to 5bed60c Compare February 12, 2025 14:05

Per Goncalves da Silva added 2 commits February 12, 2025 15:12

Decompose RegistryV1ToHelmChart function

a43ef25

Signed-off-by: Per Goncalves da Silva <pegoncal@redhat.com>

Add ParseFS failure unit tests

f02b793

Signed-off-by: Per Goncalves da Silva <pegoncal@redhat.com> Signed-off-by: Per G. da Silva <pegoncal@redhat.com>

perdasilva force-pushed the refactor-render branch from 5bed60c to f02b793 Compare February 12, 2025 14:12

camilamacedo86 approved these changes Feb 12, 2025

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 12, 2025

Merge branch 'main' into refactor-render

6cf4dbe

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Feb 12, 2025

perdasilva added this pull request to the merge queue Feb 12, 2025

Merged via the queue into operator-framework:main with commit f8fac24 Feb 12, 2025
21 of 22 checks passed

Uh oh!

🌱 Decompose RegistryV1ToHelmChart function #1735

🌱 Decompose RegistryV1ToHelmChart function #1735

Uh oh!

Conversation

perdasilva commented Feb 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Reviewer Checklist

Uh oh!

netlify bot commented Feb 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for olmv1 ready!

Uh oh!

codecov bot commented Feb 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tmshort commented Feb 10, 2025

Uh oh!

camilamacedo86 left a comment

Choose a reason for hiding this comment

Uh oh!

perdasilva commented Feb 11, 2025

Uh oh!

perdasilva Feb 11, 2025

Choose a reason for hiding this comment

Uh oh!

camilamacedo86 Feb 11, 2025

Choose a reason for hiding this comment

Uh oh!

camilamacedo86 Feb 11, 2025

Choose a reason for hiding this comment

Uh oh!

camilamacedo86 Feb 11, 2025

Choose a reason for hiding this comment

Uh oh!

camilamacedo86 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

LalatenduMohanty commented Feb 11, 2025

Uh oh!

camilamacedo86 left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Feb 12, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

perdasilva commented Feb 10, 2025 •

edited

Loading

netlify bot commented Feb 10, 2025 •

edited

Loading

codecov bot commented Feb 10, 2025 •

edited

Loading