Consolidate code from catalogd/internal in to operator-controller/internal

[LalatenduMohanty]

Currently the code of catalogd/internal and operataor-controller/internal exists separately. We need to look in to what we can consolidate in one place i.e. operataor-controller/internal

[1] https://github.com/operator-framework/operator-controller/tree/main/catalogd/internal
[2] https://github.com/operator-framework/operator-controller/tree/main/internal

[azych]

What is the target internal architecture we're looking to reach?

Is the idea to move all catalogd/internal to operator-controller and have catalogd use dependencies from there meaning it would no longer have its own /internal?
Do we want to create a /pkg or /shared (or any other) directory to store shared/exported packages? Do we want to differentiate that from shared/unexported packages?

The RFC seems to be missing those details and IMO it's important to know the specific thought-out final structure we're looking for.

[camilamacedo86]

HI @azych

The idea here is to move the pkg code from catalogd to the root (operator-controller)
we still need to use internal because we do not want to expose our code. Otherwise, we need to support all related to our internal code as well since someone could start to use it.

[LalatenduMohanty]

Let me look in to this and get back here with my suggestion , you can review and let me know if that sounds good.

[azych]

sounds great, just to clarify - I'm not saying we should have a very low-level detailed plan as in "move file X to here, export functions from file Y to here etc.", but more like general and consistent outline of the final structure that's thought-through, readable and predictable for the future.

[camilamacedo86]

It may end up being the same as what we have in the catalog, just in the root directory (all that is the internal of catalogd will be under internal oper-con). However, if we encounter any issues, we'll need to discuss them as they arise. At this point, it seems complicated to come up with more details before we try it out

Suppose we encounter duplications or other issues that block the primary goal (moving internal components from A to B). In that case, we'll handle them on a case-by-case basis as part of this initiative. However, the primary goal of this task is to move everything with minimal changes—the focus is on the move itself. Significant refactoring will probably be needed, but that should come later through multiple follow-up PRs. We need to work with baby steps so we can move faster, keep PRs small, speed up reviews, and get changes merged more quickly and make it easier the collab.

[joelanford]

Putting aside the question of how to consolidate catalogd and operator-controller, I've had a few more fundamental feelings recently:

• Do we have too many packages directly under internal?
• Are we inconsistent where some directories in internal are packages, but some are just parent directories that have packages somewhere further down?
• Do we have good names for our packages? Are they well-organized.

I agree with @azych that it might be helpful to look at everything holistically and decide if we can drive some consistency as part of this exercise:

Current listing of our internal trees (just directories):

internal
├── action
│   └── error
├── applier
├── authentication
├── bundleutil
├── catalogd
├── catalogmetadata
│   ├── cache
│   ├── client
│   ├── compare
│   └── filter
├── conditionsets
├── contentmanager
│   ├── cache
│   └── source
│       └── internal
├── controllers
├── features
├── finalizers
├── httputil
├── labels
├── resolve
├── rukpak
│   ├── convert
│   │   └── testdata
│   │       └── combine-properties-bundle
│   │           ├── manifests
│   │           └── metadata
│   ├── operator-registry
│   ├── preflights
│   │   └── crdupgradesafety
│   ├── source
│   └── util
├── scheme
├── shared
├── util
│   ├── fs
│   └── image
└── version
catalogd/internal
├── controllers
│   └── core
├── features
├── garbagecollection
├── metrics
├── serverutil
├── source
├── storage
├── version
└── webhook

[camilamacedo86]

Hi @joelanford,

The catalogd/internal/version is no longer required, right? We should now use it from oper-con, as both will have the same version.

My idea was to consolidate everything since both can use the same libs—only the controllers are specific to one or the other. For those, I was thinking of either adding a prefix to their names or creating a subdirectory to better organize them. The features also could be internal/features/catalogd.go and nternal/features/operator-controller.go.

Then, in follow-ups, we can start cleaning things up.

However, are you planning to do something like in follow-ups to break more and start by moving it to internal/catalogd for now? Or would you like to keep it there permanently?

[azych]

Thinking about the high-level structure and making some basic working assumptions, like:

• code specific to a component (catalogd or operator-controller) should be separated from code specific to the other component and generally 'live' close together with all other elements for that component (no mixing)
• unexported code should be differentiated from exported
• unexported code that's used by both components should be separated from both previous groups

Making those assumptions, a few concrete ideas that come to mind:

1. This requires the least amount of changes to the current structure:

catalogd/ (including its own catalogd/internal) - holds unexported code/objects specific to catalogd
shared/ - holds shared unexported code
pkg/ - holds exported code
internal/ - holds unexported code/objects specific to op-con

2. To me this makes more sense than 1 but I'm not sure if we should be keeping catalogd 'under' operator-controller root like it is now as it's not really a subpackage but a separate 'service'

catalogd/ - objects specific to catalogd
internal/ - holds unexported shared code and...
internal/catalogd - holds unexported code specific to catalogd
internal/operator-controller - holds unexported code specific to op-con
pkg - exported code

A variation of this could be to just have internal/catalogd and internal/operator-controller and move unexported shared code to a different location, like shared/, that's not under internal

3. I'd say this kind of separation could yet be better, because we're encapsulating all the code and other objects that are specific to a service within that service space and then just have a shared and exported directories. This could also probably be most flexible and easiest to reorganize internally for each project when we start thinking about details of internal structure which @joelanford is bringing up.

catalogd/ (including catalogd/internal) - holds unexported code/objects specific to catalogd
operator-controller/ (including operator-controller/internal) - holds unexported code/objects specific to operator-controller
internal or shared (or a better name if we can find one) - holds unexported shared code
pkg - holds exported code

[LalatenduMohanty]

After going though the existing code I think below structure would be right for the code (It is similar to @azych's suggestion) . It is logical , intuitively easy to understand and sustainable in long term. Also each step needs to be separate PRs, to reduce the chance to human error during doing the change and also during review.

catalogd/ - code specific to catalogd
internal/ - holds internal code of catalogd and operator-controller. 
internal/catalogd - holds unexported code specific to catalogd
internal/operator-controller - holds unexported code specific to operator-comntroller
internal/shared - shared code between catalogd and operator-controller which can not be exported outside.
pkg/ - exported code

Each movement of code going break existing tests. I am using a VScode plugin [1] which fixes the import path automatically but still unit tests are failing.

I am on the fence about whether all controllers should be together in once directory or they should live separately i.e. catalogd controllers in catalogd directory and operator-controller controllers in operator-controller directory. I do not think either way it is bad.

Another area is cmd (it is outside of internal code but something we can consolidate). Same with api , bin .

[1] https://marketplace.visualstudio.com/items?itemName=sleistner.vscode-fileutils

[camilamacedo86]

Hi @LalatenduMohanty,

I believe we don’t have anything that is exported today (and likely don’t want to in the future either). Do we really want to encourage using the operator-framework as a library and commit to supporting it long-term?

PS: This might make sense if we move forward with @perdasilva’s POC, but at this stage, it feels like premature optimization.

That said, I’m not quite sure why we are discussing:

pkg/ – Exported code.

For me, the relevant discussion seems to be focused only on:

• operator-controller/internal
• operator-controller/catalogd/internal

What parts of this are truly specific to catalogd and not general implementations that could serve both components?

From my perspective, only the controllers fall into this category.

We may need to discuss this further, but do we see any other cases where this applies? Perhaps for certain features, we could introduce specific functionality for one or the other; however, could we not also have the same feature gate for both projects, like MetricsFeature or something similar? It seems that we could share this as well.

I think this is the key point of the discussion that we need to explore.

• What are the code that today and always will be specific for each component?
• What are the points that are duplicated ( I,e 🌱 (cleanup): remove duplication of version implementation #1728 and https://github.com/operator-framework/operator-controller/pull/1731/files )
• Determine how we will structure the controllers? Maybe as suggested above internal/catalogd/controllers shows fine for me

Then, if ALL is mainly under internal/shared why do we need to create this structure?
Should not make more sense

internal/-> ALL that is not specific to ANY
internal/operator-controller -> ALL that is exclusive to operator-controller
internal/catalogd -> ALL that is exclusive to catalogd

PS. I am not big fan of shared dir name
IHMO the above is intuitive enough and more clean approach.

[camilamacedo86]

We discussed it today, and we agreed with the plan:

Phase 1: Initial Refactoring
The current internal structure will be adjusted as follows:

operator-controller
├── internal
│   ├── operator-controller
│   │   └── All that is in the internal today ( less what was moved to shared )
│   ├── Catalogd
│   │   └── All that is the catalogd/internal today
│   ├── Shared
│   │   └── Version, internal/util ( and all the we know that it is shared already )

Phase 2: Follow-ups
Once Phase 1 is complete, we will work on a series of PRs to:

• Consolidate duplicated internal content from the components
• Move shared logic into shared as needed.