🌱 Migrate Rukpak - Githubissues

dtfranz commented 3 months ago

Moves rukpak code imported by operator-controller into the internal/ folder.

Fixes #997

netlify[bot] commented 3 months ago

Deploy Preview for olmv1 ready!

Name	Link
Latest commit	bf85b3cb14133bd22e5daf3d3f010971b1285842
Latest deploy log	https://app.netlify.com/sites/olmv1/deploys/66914a8cf0571100098ef5c7
Deploy Preview	https://deploy-preview-1032--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.

dtfranz commented 3 months ago

General comment: there are likely some things copied over that are unused. I'll go through and make sure that's all removed.

codecov[bot] commented 3 months ago

Codecov Report

Attention: Patch coverage is 66.56347% with 216 lines in your changes missing coverage. Please review.

Project coverage is 73.46%. Comparing base (64f99f2) to head (bf85b3c).

Files	Patch %	Lines
internal/rukpak/source/image_registry.go	41.34%	44 Missing and 17 partials :warning:
internal/rukpak/convert/registryv1.go	85.10%	23 Missing and 19 partials :warning:
internal/rukpak/provisioner/plain/plain.go	60.00%	10 Missing and 10 partials :warning:
internal/rukpak/source/unpacker.go	0.00%	19 Missing :warning:
internal/rukpak/util/tar.go	47.05%	10 Missing and 8 partials :warning:
...ak/preflights/crdupgradesafety/crdupgradesafety.go	70.17%	10 Missing and 7 partials :warning:
...ternal/rukpak/bundledeployment/bundledeployment.go	0.00%	12 Missing :warning:
internal/rukpak/util/fs.go	0.00%	10 Missing :warning:
internal/rukpak/storage/localdir.go	73.33%	4 Missing and 4 partials :warning:
internal/rukpak/util/util.go	80.00%	2 Missing and 2 partials :warning:
... and 3 more

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #1032 +/- ## ========================================== - Coverage 77.00% 73.46% -3.55% ========================================== Files 19 32 +13 Lines 1357 1986 +629 ========================================== + Hits 1045 1459 +414 - Misses 222 368 +146 - Partials 90 159 +69 ``` | [Flag](https://app.codecov.io/gh/operator-framework/operator-controller/pull/1032/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=operator-framework) | Coverage Δ | | |---|---|---| | [e2e](https://app.codecov.io/gh/operator-framework/operator-controller/pull/1032/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=operator-framework) | `55.84% <55.72%> (-0.61%)` | :arrow_down: | | [unit](https://app.codecov.io/gh/operator-framework/operator-controller/pull/1032/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=operator-framework) | `45.01% <27.39%> (-8.86%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=operator-framework#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

dtfranz commented 3 months ago

General comment: there are likely some things copied over that are unused. I'll go through and make sure that's all removed.

I've removed everything I could find that was unused; see comparison here to see what I left out.

joelanford commented 3 months ago

WDYT about dumping everything in under internal/rukpak for now so that it's more obvious in a first pass what came from rukpak and what changed in existing operator-controller code?

I'm very much in favor of moving our rukpak code directly into this repo, but I'm also feeling a little bit like we need to (in a future PR) step back and take stock of our suddenly sprawling codebase to see if there are obsolete abstractions (e.g. unpack + storage) and other ways to reduce our package count and tidy things up a bit.

dtfranz commented 3 months ago

WDYT about dumping everything in under internal/rukpak for now so that it's more obvious in a first pass what came from rukpak and what changed in existing operator-controller code?

I'm very much in agreement with this and your other thought as well. Since I filtered out a lot of the types there's not currently much reason for the abstract types like storage to exist in the codebase.

For now at the very least I'll move everything into the rukpak subdir inside internal.