Skip to content

Reduce variable count #453

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Oct 12, 2023
Merged

Reduce variable count #453

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 9 additions & 16 deletions internal/resolution/variables/bundle.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,17 +26,17 @@ func (b *BundleVariable) Dependencies() []*catalogmetadata.Bundle {
return b.dependencies
}

func NewBundleVariable(id deppy.Identifier, bundle *catalogmetadata.Bundle, dependencies []*catalogmetadata.Bundle) *BundleVariable {
var dependencyIDs []deppy.Identifier
func NewBundleVariable(bundle *catalogmetadata.Bundle, dependencies []*catalogmetadata.Bundle) *BundleVariable {
dependencyIDs := make([]deppy.Identifier, 0, len(dependencies))
for _, bundle := range dependencies {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: unrelated to this PR, but we can we change the loop variable to be called dependency instead of bundle so that it doesn't shadow the bundle variable passed in as a parameter?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll submit a follow up PR straight after merging this one.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#455

dependencyIDs = append(dependencyIDs, BundleToBundleVariableIDs(bundle)...)
dependencyIDs = append(dependencyIDs, BundleVariableID(bundle))
}
var constraints []deppy.Constraint
if len(dependencyIDs) > 0 {
constraints = append(constraints, constraint.Dependency(dependencyIDs...))
}
return &BundleVariable{
SimpleVariable: input.NewSimpleVariable(id, constraints...),
SimpleVariable: input.NewSimpleVariable(BundleVariableID(bundle), constraints...),
bundle: bundle,
dependencies: dependencies,
}
Expand All @@ -59,16 +59,9 @@ func NewBundleUniquenessVariable(id deppy.Identifier, atMostIDs ...deppy.Identif
}
}

// BundleToBundleVariableIDs returns a list of all possible IDs for a bundle.
// A bundle can be present in multiple channels and we need a separate variable
// with a unique ID for each occurrence.
// Result must be deterministic.
func BundleToBundleVariableIDs(bundle *catalogmetadata.Bundle) []deppy.Identifier {
out := make([]deppy.Identifier, 0, len(bundle.InChannels))
for _, ch := range bundle.InChannels {
out = append(out, deppy.Identifier(
fmt.Sprintf("%s-%s-%s-%s", bundle.CatalogName, bundle.Package, ch.Name, bundle.Name),
))
}
return out
// BundleVariableID returns an ID for a given bundle.
func BundleVariableID(bundle *catalogmetadata.Bundle) deppy.Identifier {
return deppy.Identifier(
fmt.Sprintf("%s-%s-%s", bundle.CatalogName, bundle.Package, bundle.Name),
)
}
6 changes: 1 addition & 5 deletions internal/resolution/variables/bundle_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,11 +40,7 @@ func TestBundleVariable(t *testing.T) {
}},
},
}
ids := olmvariables.BundleToBundleVariableIDs(bundle)
if len(ids) != len(bundle.InChannels) {
t.Fatalf("bundle should produce one variable ID per channel; received: %d", len(bundle.InChannels))
}
bv := olmvariables.NewBundleVariable(ids[0], bundle, dependencies)
bv := olmvariables.NewBundleVariable(bundle, dependencies)

if bv.Bundle() != bundle {
t.Errorf("bundle '%v' does not match expected '%v'", bv.Bundle(), bundle)
Expand Down
4 changes: 2 additions & 2 deletions internal/resolution/variables/installed_package.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,9 @@ func (r *InstalledPackageVariable) Bundles() []*catalogmetadata.Bundle {

func NewInstalledPackageVariable(packageName string, bundles []*catalogmetadata.Bundle) *InstalledPackageVariable {
id := deppy.IdentifierFromString(fmt.Sprintf("installed package %s", packageName))
var variableIDs []deppy.Identifier
variableIDs := make([]deppy.Identifier, 0, len(bundles))
for _, bundle := range bundles {
variableIDs = append(variableIDs, BundleToBundleVariableIDs(bundle)...)
variableIDs = append(variableIDs, BundleVariableID(bundle))
}
return &InstalledPackageVariable{
SimpleVariable: input.NewSimpleVariable(id, constraint.Mandatory(), constraint.Dependency(variableIDs...)),
Expand Down
4 changes: 2 additions & 2 deletions internal/resolution/variables/required_package.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,9 @@ func (r *RequiredPackageVariable) Bundles() []*catalogmetadata.Bundle {

func NewRequiredPackageVariable(packageName string, bundles []*catalogmetadata.Bundle) *RequiredPackageVariable {
id := deppy.IdentifierFromString(fmt.Sprintf("required package %s", packageName))
var variableIDs []deppy.Identifier
variableIDs := make([]deppy.Identifier, 0, len(bundles))
for _, bundle := range bundles {
variableIDs = append(variableIDs, BundleToBundleVariableIDs(bundle)...)
variableIDs = append(variableIDs, BundleVariableID(bundle))
}
return &RequiredPackageVariable{
SimpleVariable: input.NewSimpleVariable(id, constraint.Mandatory(), constraint.Dependency(variableIDs...)),
Expand Down
41 changes: 20 additions & 21 deletions internal/resolution/variablesources/bundles_and_dependencies.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,25 +63,25 @@ func (b *BundlesAndDepsVariableSource) GetVariables(ctx context.Context) ([]depp
var head *catalogmetadata.Bundle
head, bundleQueue = bundleQueue[0], bundleQueue[1:]

for _, id := range olmvariables.BundleToBundleVariableIDs(head) {
// ignore bundles that have already been processed
if _, ok := visited[id]; ok {
continue
}
visited[id] = struct{}{}

// get bundle dependencies
dependencies, err := b.filterBundleDependencies(allBundles, head)
if err != nil {
return nil, fmt.Errorf("could not determine dependencies for bundle with id '%s': %w", id, err)
}
id := olmvariables.BundleVariableID(head)

// add bundle dependencies to queue for processing
bundleQueue = append(bundleQueue, dependencies...)
// ignore bundles that have already been processed
if _, ok := visited[id]; ok {
continue
}
visited[id] = struct{}{}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use sets.New[deppy.Identifier]() for clearer semantics on visited and added?

Copy link
Member Author

@m1kola m1kola Oct 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK Go doesn't have sets in standard library and as a result using maps for sets is pretty common.

But we can do that. However I do not see any sets package in our codebase. What package is it?

Edit: is it k8s.io/apimachinery/pkg/util/sets?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anyway I think it should not be in this PR. Could you please create an issue or PR for this suggesting a pacakge to use?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The k8s API machinery sets package is the one - we use it in basically every other package. Feel free to create the issue if you don't think you can do it here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a fan of small PRs serve one porpuse. Don't want to mix different efforts into this PR.

Created #454 for this.


// create variable
variables = append(variables, olmvariables.NewBundleVariable(id, head, dependencies))
// get bundle dependencies
dependencies, err := b.filterBundleDependencies(allBundles, head)
if err != nil {
return nil, fmt.Errorf("could not determine dependencies for bundle with id '%s': %w", id, err)
}

// add bundle dependencies to queue for processing
bundleQueue = append(bundleQueue, dependencies...)

// create variable
variables = append(variables, olmvariables.NewBundleVariable(head, dependencies))
}

return variables, nil
Expand All @@ -101,11 +101,10 @@ func (b *BundlesAndDepsVariableSource) filterBundleDependencies(allBundles []*ca
}
for i := 0; i < len(packageDependencyBundles); i++ {
bundle := packageDependencyBundles[i]
for _, id := range olmvariables.BundleToBundleVariableIDs(bundle) {
if _, ok := added[id]; !ok {
dependencies = append(dependencies, bundle)
added[id] = struct{}{}
}
id := olmvariables.BundleVariableID(bundle)
if _, ok := added[id]; !ok {
dependencies = append(dependencies, bundle)
added[id] = struct{}{}
}
}
}
Expand Down
16 changes: 8 additions & 8 deletions internal/resolution/variablesources/bundles_and_dependencies_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -316,13 +316,13 @@ var _ = Describe("BundlesAndDepsVariableSource", func() {
// dependencies (bundles 7, 8, 9, 10, 11) to appear here due to their GVKs being required by
// some of the packages.
Expect(bundleVariables).To(WithTransform(CollectBundleVariableIDs, Equal([]string{
"fake-catalog-test-package-stable-bundle-2",
"fake-catalog-test-package-stable-bundle-1",
"fake-catalog-test-package-2-stable-bundle-15",
"fake-catalog-test-package-2-stable-bundle-16",
"fake-catalog-test-package-2-stable-bundle-17",
"fake-catalog-some-package-stable-bundle-5",
"fake-catalog-some-package-stable-bundle-4",
"fake-catalog-test-package-bundle-2",
"fake-catalog-test-package-bundle-1",
"fake-catalog-test-package-2-bundle-15",
"fake-catalog-test-package-2-bundle-16",
"fake-catalog-test-package-2-bundle-17",
"fake-catalog-some-package-bundle-5",
"fake-catalog-some-package-bundle-4",
})))

// check dependencies for one of the bundles
Expand Down Expand Up @@ -374,7 +374,7 @@ var _ = Describe("BundlesAndDepsVariableSource", func() {
)
_, err := bdvs.GetVariables(context.TODO())
Expect(err).To(HaveOccurred())
Expect(err.Error()).To(ContainSubstring("could not determine dependencies for bundle with id 'fake-catalog-test-package-stable-bundle-2': could not find package dependencies for bundle 'bundle-2'"))
Expect(err.Error()).To(ContainSubstring("could not determine dependencies for bundle with id 'fake-catalog-test-package-bundle-2': could not find package dependencies for bundle 'bundle-2'"))
})

It("should return error if an inner variable source returns an error", func() {
Expand Down
13 changes: 6 additions & 7 deletions internal/resolution/variablesources/crd_constraints.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,15 +50,14 @@ func (g *CRDUniquenessConstraintsVariableSource) GetVariables(ctx context.Contex
bundles := []*catalogmetadata.Bundle{v.Bundle()}
bundles = append(bundles, v.Dependencies()...)
for _, bundle := range bundles {
for _, id := range olmvariables.BundleToBundleVariableIDs(bundle) {
// get bundleID package and update map
packageName := bundle.Package
id := olmvariables.BundleVariableID(bundle)
// get bundleID package and update map
packageName := bundle.Package

if _, ok := pkgToBundleMap[packageName]; !ok {
pkgToBundleMap[packageName] = map[deppy.Identifier]struct{}{}
}
pkgToBundleMap[packageName][id] = struct{}{}
if _, ok := pkgToBundleMap[packageName]; !ok {
pkgToBundleMap[packageName] = map[deppy.Identifier]struct{}{}
}
pkgToBundleMap[packageName][id] = struct{}{}
}
}
}
Expand Down
26 changes: 13 additions & 13 deletions internal/resolution/variablesources/crd_constraints_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,7 @@ var _ = Describe("CRDUniquenessConstraintsVariableSource", func() {
bundleSet["bundle-15"],
bundleSet["bundle-16"],
}),
olmvariables.NewBundleVariable(olmvariables.BundleToBundleVariableIDs(bundleSet["bundle-2"])[0],
olmvariables.NewBundleVariable(
bundleSet["bundle-2"],
[]*catalogmetadata.Bundle{
bundleSet["bundle-3"],
Expand All @@ -213,59 +213,59 @@ var _ = Describe("CRDUniquenessConstraintsVariableSource", func() {
bundleSet["bundle-7"],
},
),
olmvariables.NewBundleVariable(olmvariables.BundleToBundleVariableIDs(bundleSet["bundle-1"])[0],
olmvariables.NewBundleVariable(
bundleSet["bundle-1"],
[]*catalogmetadata.Bundle{
bundleSet["bundle-6"],
bundleSet["bundle-7"],
bundleSet["bundle-8"],
},
),
olmvariables.NewBundleVariable(olmvariables.BundleToBundleVariableIDs(bundleSet["bundle-3"])[0],
olmvariables.NewBundleVariable(
bundleSet["bundle-3"],
[]*catalogmetadata.Bundle{},
),
olmvariables.NewBundleVariable(olmvariables.BundleToBundleVariableIDs(bundleSet["bundle-4"])[0],
olmvariables.NewBundleVariable(
bundleSet["bundle-4"],
[]*catalogmetadata.Bundle{},
),
olmvariables.NewBundleVariable(olmvariables.BundleToBundleVariableIDs(bundleSet["bundle-5"])[0],
olmvariables.NewBundleVariable(
bundleSet["bundle-5"],
[]*catalogmetadata.Bundle{},
),
olmvariables.NewBundleVariable(olmvariables.BundleToBundleVariableIDs(bundleSet["bundle-6"])[0],
olmvariables.NewBundleVariable(
bundleSet["bundle-6"],
[]*catalogmetadata.Bundle{},
),
olmvariables.NewBundleVariable(olmvariables.BundleToBundleVariableIDs(bundleSet["bundle-7"])[0],
olmvariables.NewBundleVariable(
bundleSet["bundle-7"],
[]*catalogmetadata.Bundle{
bundleSet["bundle-8"],
bundleSet["bundle-9"],
bundleSet["bundle-10"],
},
),
olmvariables.NewBundleVariable(olmvariables.BundleToBundleVariableIDs(bundleSet["bundle-8"])[0],
olmvariables.NewBundleVariable(
bundleSet["bundle-8"],
[]*catalogmetadata.Bundle{},
),
olmvariables.NewBundleVariable(olmvariables.BundleToBundleVariableIDs(bundleSet["bundle-9"])[0],
olmvariables.NewBundleVariable(
bundleSet["bundle-9"],
[]*catalogmetadata.Bundle{},
),
olmvariables.NewBundleVariable(olmvariables.BundleToBundleVariableIDs(bundleSet["bundle-10"])[0],
olmvariables.NewBundleVariable(
bundleSet["bundle-10"],
[]*catalogmetadata.Bundle{},
),
olmvariables.NewBundleVariable(olmvariables.BundleToBundleVariableIDs(bundleSet["bundle-14"])[0],
olmvariables.NewBundleVariable(
bundleSet["bundle-14"],
[]*catalogmetadata.Bundle{},
),
olmvariables.NewBundleVariable(olmvariables.BundleToBundleVariableIDs(bundleSet["bundle-15"])[0],
olmvariables.NewBundleVariable(
bundleSet["bundle-15"],
[]*catalogmetadata.Bundle{},
),
olmvariables.NewBundleVariable(olmvariables.BundleToBundleVariableIDs(bundleSet["bundle-16"])[0],
olmvariables.NewBundleVariable(
bundleSet["bundle-16"],
[]*catalogmetadata.Bundle{},
),
Expand Down