Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reduce number of variable sources involved in resolution process #437

Closed
m1kola opened this issue Oct 5, 2023 · 4 comments · Fixed by #501
Closed

Reduce number of variable sources involved in resolution process #437

m1kola opened this issue Oct 5, 2023 · 4 comments · Fixed by #501
Assignees

Comments

@m1kola
Copy link
Member

m1kola commented Oct 5, 2023

Today we have variable sources which create other variable sources which create variables. While this gives us very short and simple units - it makes harder to follow and creates a lot of boilerplate (e.g. need for support of nested variable soruces).

For example OperatorVariableSource creates RequiredPackageVariableSource which creates RequiredPackageVariable:

rps, err := NewRequiredPackageVariableSource(
o.catalogClient,
operator.Spec.PackageName,
InVersionRange(operator.Spec.Version),
InChannel(operator.Spec.Channel),
)

return []deppy.Variable{
olmvariables.NewRequiredPackageVariable(r.packageName, resultSet),
}, nil

BundleDeploymentVariableSource creates InstalledPackageVariableSource which creates InstalledPackageVariable:

ips, err := NewInstalledPackageVariableSource(o.catalogClient, bundleDeployment.Spec.Template.Spec.Source.Image.Ref)

return []deppy.Variable{
variables.NewInstalledPackageVariable(installedBundle.Package, upgradeEdges),
}, nil

@m1kola
Copy link
Member Author

m1kola commented Oct 5, 2023

We can start by reducing number of variable sources so that variable sources don't create other variable sources.

We might still have to preserve custom variable types such as InstalledPackageVariable and RequiredPackageVariable since we use custom variable types to store references to bundles which are later used in other variable sources:

switch v := variable.(type) {
case *olmvariables.RequiredPackageVariable:
bundleQueue = append(bundleQueue, v.Bundles()...)
case *olmvariables.InstalledPackageVariable:
bundleQueue = append(bundleQueue, v.Bundles()...)
}

But this might be at some point resolved too. Soon we are going to be looking into potential next iterations for deppy and one of the ideas which was mentioned is adding "kind" and attributes to variables. This might be a good use case for it. But this still need to be fleshed out.

@m1kola
Copy link
Member Author

m1kola commented Oct 10, 2023

Last week were discussing something remotely related and touched on variable sources structure. There was an idea to have one variable source but split the work it does into smaller functions/structs.

This will give us small unit testable pieces of code and should help readability as today it is hard to follow what is going on during the variable gathering stage.

@perdasilva, @joelanford and @dtfranz please correct me if I captured the idea incorrectly and feel free to add any context to it.

I still think the first iteration can be just getting rid of variable sources that produce other variables sources as suggested above. Once we complete this step it will be easy to split into functions.

@perdasilva
Copy link
Contributor

I think the general idea is to move to a monolith variable source that handles everything. The idea of introducing something like group/version/kind to Variables could help obviate the need to make your own implementations of the variable interface

@m1kola m1kola self-assigned this Oct 23, 2023
@m1kola m1kola changed the title Reduce number of variable sources and variable types involved in resolution process Reduce number of variable sources involved in resolution process Oct 31, 2023
@m1kola
Copy link
Member Author

m1kola commented Oct 31, 2023

I updated this issue to focus on reducing number of varaible sources and created a separate issue for reducing number of variable types: #494

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants