Skip to content

Commit

Permalink
fix: add checking if error is new in dependency validation (#737)
Browse files Browse the repository at this point in the history 
Signed-off-by: Jakob Steiner <[email protected]>
  • Loading branch information
@kosmoz
kosmoz authored Jun 3, 2024
1 parent 298e8f5 commit fcf21ca
Show file tree
Hide file tree
Showing 2 changed files with 88 additions and 68 deletions.
29 changes: 24 additions & 5 deletions internal/dependency/manager.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,9 +43,12 @@ func (dm *DependendcyManager) Validate(
return nil, err
}

// We can not call validate the graph here, because the initial graph, representing the current cluster state, may
// We do not check the validation error here, because the initial graph, representing the current cluster state, may
// actually be invalid. This is because we let the operator create/update dependencies which can only happen after
// a package is already created.
// We need this error though in order to check later whether a dependency error was introduced by the action that
// is currently validated or existed before.
errBefore := g.Validate()

if err := dm.add(g, *manifest, version); err != nil {
return nil, err
Expand All @@ -59,10 +62,12 @@ func (dm *DependendcyManager) Validate(

var conflicts []Conflict
for _, err := range multierr.Errors(g.Validate()) {
if conflict, err := errorToConflict(err); err != nil {
return nil, err
} else {
conflicts = append(conflicts, *conflict)
if isErrNew(err, errBefore) {
if conflict, err := errorToConflict(err); err != nil {
return nil, err
} else {
conflicts = append(conflicts, *conflict)
}
}
}

Expand Down Expand Up @@ -169,6 +174,20 @@ func errorToConflict(err error) (*Conflict, error) {
}
}

func isErrNew(errCurrent error, errBefore error) bool {
if errCurrentDep := (&graph.DependencyError{}); errors.As(errCurrent, &errCurrentDep) {
for _, err := range multierr.Errors(errBefore) {
if errBeforeDep := (&graph.DependencyError{}); errors.As(err, &errBeforeDep) &&
errBeforeDep.Name == errCurrentDep.Name &&
errBeforeDep.Dependency == errCurrentDep.Dependency {
return false
}

}
}
return true
}

// getVersions is a utility to get all versions for a package from repoAdapter and also parse them
func (dm *DependendcyManager) getVersions(name string) ([]*semver.Version, error) {
if versions, err := dm.repoAdapter.GetVersions(name); err != nil {
Expand Down
127 changes: 64 additions & 63 deletions internal/dependency/manager_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,15 @@ package dependency

import (
"context"
"strings"

"github.com/glasskube/glasskube/api/v1alpha1"
"github.com/glasskube/glasskube/internal/names"
"github.com/glasskube/glasskube/internal/repo/client/fake"
. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime/schema"
)

var fakeRepo = &fake.FakeClient{
Expand All @@ -18,34 +20,21 @@ var fakeRepo = &fake.FakeClient{
}

type testClientAdapter struct {
packages []v1alpha1.Package
packageInfos []v1alpha1.PackageInfo
}

func (a *testClientAdapter) GetPackageInfo(ctx context.Context, pkgInfoName string) (*v1alpha1.PackageInfo, error) {
if strings.HasPrefix(pkgInfoName, "p") && pi != nil {
return pi, nil
} else if strings.HasPrefix(pkgInfoName, "d") && di != nil {
return di, nil
} else if strings.HasPrefix(pkgInfoName, "e") && ei != nil {
return ei, nil
} else {
return nil, nil
for _, pi := range a.packageInfos {
if pkgInfoName == pi.Name {
return &pi, nil
}
}
return nil, errors.NewNotFound(schema.GroupResource{}, pkgInfoName)
}

func (a *testClientAdapter) ListPackages(ctx context.Context) (*v1alpha1.PackageList, error) {
var items []v1alpha1.Package
if p != nil {
items = append(items, *p)
}
if d != nil {
items = append(items, *d)
}
if e != nil {
items = append(items, *e)
}
return &v1alpha1.PackageList{
Items: items,
}, nil
return &v1alpha1.PackageList{Items: a.packages}, nil
}

// GetPackage implements adapter.PackageClientAdapter.
Expand All @@ -65,9 +54,11 @@ func (a *testClientAdapter) ListPackageRepositories(ctx context.Context) (*v1alp
}

func createDependencyManager() *DependendcyManager {
return NewDependencyManager(&testClientAdapter{}, &fake.FakeClientset{Client: fakeRepo})
testClient = &testClientAdapter{}
return NewDependencyManager(testClient, &fake.FakeClientset{Client: fakeRepo})
}

var testClient *testClientAdapter
var dm *DependendcyManager

// For the following test suite, we always use the Package p (name "P") as the package who's dependencies should be
Expand All @@ -80,39 +71,49 @@ var d, e, p, x, y *v1alpha1.Package
// di, ei, pi, xi, yi are the corresponding PackageInfo's to the package d, p, x, y
var di, ei, pi, xi, yi *v1alpha1.PackageInfo

func createPackageAndInfo(name string, version string) (*v1alpha1.Package, *v1alpha1.PackageInfo) {
func createPackageAndInfo(name string, version string, installed bool) (*v1alpha1.Package, *v1alpha1.PackageInfo) {
manifest := v1alpha1.PackageManifest{
Name: name,
}
fakeRepo.AddPackage(name, version, &manifest)
return &v1alpha1.Package{
ObjectMeta: v1.ObjectMeta{
Name: name,
},
Spec: v1alpha1.PackageSpec{
PackageInfo: v1alpha1.PackageInfoTemplate{
Name: name,
Version: version,
},
},
Status: v1alpha1.PackageStatus{OwnedPackageInfos: []v1alpha1.OwnedResourceRef{{Name: name}}},
}, &v1alpha1.PackageInfo{
Spec: v1alpha1.PackageInfoSpec{
pkg := v1alpha1.Package{
ObjectMeta: metav1.ObjectMeta{
Name: name,
},
Spec: v1alpha1.PackageSpec{
PackageInfo: v1alpha1.PackageInfoTemplate{
Name: name,
Version: version,
},
Status: v1alpha1.PackageInfoStatus{
Version: version,
Manifest: &manifest,
},
}
},
Status: v1alpha1.PackageStatus{OwnedPackageInfos: []v1alpha1.OwnedResourceRef{{Name: name}}},
}

pkgi := v1alpha1.PackageInfo{
ObjectMeta: metav1.ObjectMeta{
Name: names.PackageInfoName(pkg),
},
Spec: v1alpha1.PackageInfoSpec{
Name: name,
Version: version,
},
Status: v1alpha1.PackageInfoStatus{
Version: version,
Manifest: &manifest,
},
}
if installed {
testClient.packages = append(testClient.packages, pkg)
testClient.packageInfos = append(testClient.packageInfos, pkgi)
}
return &pkg, &pkgi
}

var _ = Describe("Dependency Manager", func() {

BeforeEach(func() {
p, pi = createPackageAndInfo("P", "12.2.0")
dm = createDependencyManager()
p, pi = createPackageAndInfo("P", "12.2.0", false)
})

AfterEach(func() {
Expand Down Expand Up @@ -156,7 +157,7 @@ var _ = Describe("Dependency Manager", func() {
When("D exists", func() {

BeforeEach(func() {
d, di = createPackageAndInfo("D", "1.1.1")
d, di = createPackageAndInfo("D", "1.1.1", true)
})

When("no other package dependent on D", func() {
Expand All @@ -173,11 +174,11 @@ var _ = Describe("Dependency Manager", func() {
When("other existing packages X, Y dependent on D", func() {

BeforeEach(func() {
x, xi = createPackageAndInfo("X", "0.17.2")
x, xi = createPackageAndInfo("X", "0.17.2", true)
xi.Status.Manifest.Dependencies = []v1alpha1.Dependency{{
Name: "D",
}}
y, yi = createPackageAndInfo("Y", "3.2.0-beta.7")
y, yi = createPackageAndInfo("Y", "3.2.0-beta.7", true)
yi.Status.Manifest.Dependencies = []v1alpha1.Dependency{{
Name: "D",
}}
Expand Down Expand Up @@ -265,11 +266,11 @@ var _ = Describe("Dependency Manager", func() {
When("there is no other existing package dependent on D", func() {

BeforeEach(func() {
x, xi = createPackageAndInfo("X", "0.17.0") // X here has no dependency on D
x, xi = createPackageAndInfo("X", "0.17.0", true) // X here has no dependency on D
})

It("should return OK if D's version is in required range", func(ctx context.Context) {
d, di = createPackageAndInfo("D", "1.3.0")
d, di = createPackageAndInfo("D", "1.3.0", true)
res, err := dm.Validate(ctx, pi.Status.Manifest, p.Spec.PackageInfo.Version)
Expect(err).ShouldNot(HaveOccurred())
Expect(res).ShouldNot(BeNil())
Expand All @@ -279,7 +280,7 @@ var _ = Describe("Dependency Manager", func() {
})

It("should return CONFLICT if D's version is too old", func(ctx context.Context) {
d, di = createPackageAndInfo("D", "1.2.1")
d, di = createPackageAndInfo("D", "1.2.1", true)
res, err := dm.Validate(ctx, pi.Status.Manifest, p.Spec.PackageInfo.Version)
Expect(err).ShouldNot(HaveOccurred())
Expect(res).ShouldNot(BeNil())
Expand All @@ -294,7 +295,7 @@ var _ = Describe("Dependency Manager", func() {
})

It("should return CONFLICT if D's version is too new", func(ctx context.Context) {
d, di = createPackageAndInfo("D", "2.0.0-alpha.2")
d, di = createPackageAndInfo("D", "2.0.0-alpha.2", true)
res, err := dm.Validate(ctx, pi.Status.Manifest, p.Spec.PackageInfo.Version)
Expect(err).ShouldNot(HaveOccurred())
Expect(res).ShouldNot(BeNil())
Expand All @@ -312,11 +313,11 @@ var _ = Describe("Dependency Manager", func() {
When("existing packages X and Y are dependent on D but require no version range of D", func() {

BeforeEach(func() {
x, xi = createPackageAndInfo("X", "0.17.3")
x, xi = createPackageAndInfo("X", "0.17.3", true)
xi.Status.Manifest.Dependencies = []v1alpha1.Dependency{{
Name: "D",
}}
y, yi = createPackageAndInfo("Y", "3.2.0-beta.8")
y, yi = createPackageAndInfo("Y", "3.2.0-beta.8", true)
yi.Status.Manifest.Dependencies = []v1alpha1.Dependency{{
Name: "D",
}}
Expand All @@ -325,7 +326,7 @@ var _ = Describe("Dependency Manager", func() {
// these are the same tests as in the previous When("there is no other existing package dependent on D")

It("should return OK if D's version is in required range", func(ctx context.Context) {
d, di = createPackageAndInfo("D", "1.3.1")
d, di = createPackageAndInfo("D", "1.3.1", true)
res, err := dm.Validate(ctx, pi.Status.Manifest, p.Spec.PackageInfo.Version)
Expect(err).ShouldNot(HaveOccurred())
Expect(res).ShouldNot(BeNil())
Expand All @@ -335,7 +336,7 @@ var _ = Describe("Dependency Manager", func() {
})

It("should return CONFLICT if D's version is too old", func(ctx context.Context) {
d, di = createPackageAndInfo("D", "1.1.7")
d, di = createPackageAndInfo("D", "1.1.7", true)
res, err := dm.Validate(ctx, pi.Status.Manifest, p.Spec.PackageInfo.Version)
Expect(err).ShouldNot(HaveOccurred())
Expect(res).ShouldNot(BeNil())
Expand All @@ -350,7 +351,7 @@ var _ = Describe("Dependency Manager", func() {
})

It("should return CONFLICT if D's version is too new", func(ctx context.Context) {
d, di = createPackageAndInfo("D", "2.0.0-alpha.2")
d, di = createPackageAndInfo("D", "2.0.0-alpha.2", true)
res, err := dm.Validate(ctx, pi.Status.Manifest, p.Spec.PackageInfo.Version)
Expect(err).ShouldNot(HaveOccurred())
Expect(res).ShouldNot(BeNil())
Expand All @@ -369,20 +370,20 @@ var _ = Describe("Dependency Manager", func() {
When("other existing packages X, Y are dependent on D and require D in version ranges", func() {

BeforeEach(func() {
x, xi = createPackageAndInfo("X", "0.18.3")
x, xi = createPackageAndInfo("X", "0.18.3", true)
xi.Status.Manifest.Dependencies = []v1alpha1.Dependency{{
Name: "D",
Version: "^1.0.0 || 2.0.0",
}}
y, yi = createPackageAndInfo("X", "3.3.3")
y, yi = createPackageAndInfo("X", "3.3.3", true)
yi.Status.Manifest.Dependencies = []v1alpha1.Dependency{{
Name: "D",
Version: ">= 1.1.0, < 3",
}}
})

It("should return OK if D's version is in required range", func(ctx context.Context) {
d, di = createPackageAndInfo("D", "1.4.0")
d, di = createPackageAndInfo("D", "1.4.0", true)
res, err := dm.Validate(ctx, pi.Status.Manifest, p.Spec.PackageInfo.Version)
Expect(err).ShouldNot(HaveOccurred())
Expect(res).ShouldNot(BeNil())
Expand All @@ -392,7 +393,7 @@ var _ = Describe("Dependency Manager", func() {
})

It("should return CONFLICT if D's version is too old", func(ctx context.Context) {
d, di = createPackageAndInfo("D", "1.2.1")
d, di = createPackageAndInfo("D", "1.2.1", true)
res, err := dm.Validate(ctx, pi.Status.Manifest, p.Spec.PackageInfo.Version)
Expect(err).ShouldNot(HaveOccurred())
Expect(res).ShouldNot(BeNil())
Expand All @@ -407,7 +408,7 @@ var _ = Describe("Dependency Manager", func() {
})

It("should return CONFLICT if D's version is too new", func(ctx context.Context) {
d, di = createPackageAndInfo("D", "2.0.0")
d, di = createPackageAndInfo("D", "2.0.0", true)
res, err := dm.Validate(ctx, pi.Status.Manifest, p.Spec.PackageInfo.Version)
Expect(err).ShouldNot(HaveOccurred())
Expect(res).ShouldNot(BeNil())
Expand Down Expand Up @@ -457,8 +458,8 @@ var _ = Describe("Dependency Manager", func() {
When("P requires no version ranges of D and E", func() {
When("D, E exist", func() {
It("Should return OK", func(ctx context.Context) {
d, di = createPackageAndInfo("D", "118.0.0")
e, ei = createPackageAndInfo("E", "11.80.0")
d, di = createPackageAndInfo("D", "118.0.0", true)
e, ei = createPackageAndInfo("E", "11.80.0", true)
res, err := dm.Validate(ctx, pi.Status.Manifest, p.Spec.PackageInfo.Version)
Expect(err).ShouldNot(HaveOccurred())
Expect(res).ShouldNot(BeNil())
Expand Down

0 comments on commit fcf21ca

Please sign in to comment.