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.

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

Jump to bottom

Handle channelName #172

Merged
merged 6 commits into from
Apr 21, 2023
Merged

Handle channelName #172

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2efcc9d
Integrate channelName
tmshort Apr 18, 2023
4ddf729
Add channelName unit tests
tmshort Apr 18, 2023
1493163
Add more tests
tmshort Apr 19, 2023
5067bfc
Update error messages
tmshort Apr 19, 2023
a93f137
Update channel name regex
tmshort Apr 19, 2023
1c538f0
Look for BundleDeployments on success
tmshort Apr 19, 2023
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
2 changes: 2 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ install.sh
*.swp
*.swo
*~
\#*\#
.\#*

# TODO dfranz remove this line and the bin folder when tools binaries are moved to their own folder
!bin/.dockerignore
27 changes: 27 additions & 0 deletions controllers/admission_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,4 +72,31 @@ var _ = Describe("Operator Spec Validations", func() {
Expect(err.Error()).To(ContainSubstring("spec.version in body should match '^(0|[1-9]\\d*)\\.(0|[1-9]\\d*)\\.(0|[1-9]\\d*)(-(0|[1-9]\\d*|[0-9]*[a-zA-Z-][0-9a-zA-Z-]*)(\\.(0|[1-9]\\d*|[0-9]*[a-zA-Z-][0-9a-zA-Z-]*))*)?(\\+([0-9a-zA-Z-]+(\\.[0-9a-zA-Z-]+)*))?$'"))
}
})
It("should fail if an invalid channel name is given", func() {
invalidChannels := []string{
"spaces spaces",
"Capitalized",
"camelCase",
"many/invalid$characters+in_name",
"-start-with-hyphen",
"end-with-hyphen-",
"channel-has-version-1.0.1",
}
for _, invalidChannel := range invalidChannels {
err := cl.Create(ctx, operator(operatorsv1alpha1.OperatorSpec{
PackageName: "package",
Channel: invalidChannel,
}))
Expect(err).To(HaveOccurred(), "expected error for invalid channel '%q'", invalidChannel)
Expect(err.Error()).To(ContainSubstring("spec.channel in body should match '^[a-z0-9]+(-[a-z0-9]+)*$'"))
}
})
It("should fail if an invalid channel name length", func() {
err := cl.Create(ctx, operator(operatorsv1alpha1.OperatorSpec{
PackageName: "package",
Channel: "longname01234567890123456789012345678901234567890",
}))
Expect(err).To(HaveOccurred(), "expected error for invalid channel length")
Expect(err.Error()).To(ContainSubstring("spec.channel: Too long: may not be longer than 48"))
})
})
78 changes: 76 additions & 2 deletions controllers/operator_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ var _ = Describe("Operator Controller Test", func() {
var pkgName string
BeforeEach(func() {
By("initializing cluster state")
pkgName = fmt.Sprintf("exists-%s", rand.String(6))
pkgName = "prometheus"
operator = &operatorsv1alpha1.Operator{
ObjectMeta: metav1.ObjectMeta{Name: opKey.Name},
Spec: operatorsv1alpha1.OperatorSpec{
Expand Down Expand Up @@ -594,6 +594,80 @@ var _ = Describe("Operator Controller Test", func() {
})

})
When("the operator specifies a channel that exist", func() {
var pkgName string
var pkgVer string
var pkgChan string
BeforeEach(func() {
By("initializing cluster state")
pkgName = "prometheus"
pkgVer = "0.47.0"
Copy link
Member

Choose a reason for hiding this comment

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

Should we leave the version unset for the base spec.channel tests so we can isolate the behavior we're expecting and avoid side effects from setting the version?

(We should probably also have tests where both are set to test for interactions, but I think that's outside the scope of this PR).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's easy enough to copy/paste the test w/o the package version.

pkgChan = "beta"
operator = &operatorsv1alpha1.Operator{
ObjectMeta: metav1.ObjectMeta{Name: opKey.Name},
Spec: operatorsv1alpha1.OperatorSpec{
PackageName: pkgName,
Version: pkgVer,
Channel: pkgChan,
},
}
err := cl.Create(ctx, operator)
Expect(err).NotTo(HaveOccurred())
})
It("sets resolution success status", func() {
By("running reconcile")
res, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: opKey})
Expect(res).To(Equal(ctrl.Result{}))
Expect(err).NotTo(HaveOccurred())

By("fetching updated operator after reconcile")
Expect(cl.Get(ctx, opKey, operator)).NotTo(HaveOccurred())

By("checking the expected conditions")
cond := apimeta.FindStatusCondition(operator.Status.Conditions, operatorsv1alpha1.TypeReady)
Expect(cond).NotTo(BeNil())
Expect(cond.Status).To(Equal(metav1.ConditionUnknown))
Expect(cond.Reason).To(Equal(operatorsv1alpha1.ReasonInstallationStatusUnknown))
Expect(cond.Message).To(ContainSubstring("waiting for BundleDeployment"))
})
})
When("the operator specifies a channel that does not exist", func() {
var pkgName string
var pkgVer string
var pkgChan string
BeforeEach(func() {
By("initializing cluster state")
pkgName = "prometheus"
pkgVer = "0.47.0"
pkgChan = "alpha"
operator = &operatorsv1alpha1.Operator{
ObjectMeta: metav1.ObjectMeta{Name: opKey.Name},
Spec: operatorsv1alpha1.OperatorSpec{
PackageName: pkgName,
Version: pkgVer,
Channel: pkgChan,
},
}
err := cl.Create(ctx, operator)
Expect(err).NotTo(HaveOccurred())
})
It("sets resolution failure status", func() {
By("running reconcile")
res, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: opKey})
Expect(res).To(Equal(ctrl.Result{}))
Expect(err).To(MatchError(fmt.Sprintf("package '%s' at version '%s' not found", pkgName, pkgVer)))

By("fetching updated operator after reconcile")
Expect(cl.Get(ctx, opKey, operator)).NotTo(HaveOccurred())

By("checking the expected conditions")
cond := apimeta.FindStatusCondition(operator.Status.Conditions, operatorsv1alpha1.TypeReady)
Expect(cond).NotTo(BeNil())
Expect(cond.Status).To(Equal(metav1.ConditionFalse))
Expect(cond.Reason).To(Equal(operatorsv1alpha1.ReasonResolutionFailed))
Expect(cond.Message).To(Equal(fmt.Sprintf("package '%s' at version '%s' not found", pkgName, pkgVer)))
Copy link
Member

Choose a reason for hiding this comment

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

This seems like a confusing message. The version exists, but just not in this channel. Do we need to update the notFoundError() in required_package.go to make the error more clear?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I though the message ought to include the channel as well, that might require updating other tests.

Copy link
Member

Choose a reason for hiding this comment

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

Based on how notFoundError() appears to work currently, the version aspect of the error only shows up when the version is set. I figure we can do a similar thing for channels.

We should handle both being set as well and try to come up with a coherent message in that case, but again, not super concerned with that in this particular PR. Do it if it's easy, but if handling it seems hard or contentious, then add a TODO comment and GH issue and move on without it. My two cents at least.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

})
})
AfterEach(func() {
verifyInvariants(ctx, operator)

Expand Down Expand Up @@ -648,7 +722,7 @@ var _ = Describe("Operator Controller Test", func() {
Expect(cond.Reason).To(Equal(operatorsv1alpha1.ReasonInvalidSpec))
Expect(cond.Message).To(Equal("invalid .spec.version: Invalid character(s) found in prerelease \"123abc_def\""))
})
})
})
})

func verifyInvariants(ctx context.Context, op *operatorsv1alpha1.Operator) {
Expand Down
3 changes: 3 additions & 0 deletions internal/resolution/variable_sources/olm/olm.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,5 +46,8 @@ func (o *OLMVariableSource) requiredPackageFromOperator(operator *operatorsv1alp
if operator.Spec.Version != "" {
opts = append(opts, required_package.InVersionRange(operator.Spec.Version))
}
if operator.Spec.Channel != "" {
opts = append(opts, required_package.InChannel(operator.Spec.Channel))
}
return required_package.NewRequiredPackage(operator.Spec.PackageName, opts...)
}
11 changes: 11 additions & 0 deletions internal/resolution/variable_sources/required_package/required_package.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,9 +54,20 @@ func InVersionRange(versionRange string) RequiredPackageOption {
}
}

func InChannel(channelName string) RequiredPackageOption {
return func(r *RequiredPackageVariableSource) error {
if channelName != "" {
r.channelName = channelName
r.predicates = append(r.predicates, predicates.InChannel(channelName))
}
return nil
}
}

type RequiredPackageVariableSource struct {
packageName string
versionRange string
channelName string
predicates []input.Predicate
}

Expand Down