Handle channelName #172
Handle channelName #172
Conversation
Signed-off-by: Todd Short <[email protected]>
Signed-off-by: Todd Short <[email protected]>
It looks like opm would have an issue with the regex w.r.t. semver catalog template generating minor version channels: https://olm.operatorframework.io/docs/reference/catalog-templates/#example-1 (actually, the last output block). For e.g. ---
entries:
- name: testoperator.v1.0.1
name: fast-v1.0
package: testoperator
schema: olm.channel
---
entries:
- name: testoperator.v1.1.0
replaces: testoperator.v1.0.1
name: fast-v1.1
package: testoperator
schema: olm.channel
---
entries:
- name: testoperator.v1.0.1
name: stable-v1.0
package: testoperator
schema: olm.channel |
There was a problem hiding this comment.
Just a few (hopefully small) suggestions, but I think this looks good overall.
I opened a follow-up issue (#176) that captures (as best I can) the topic I brought up in the WG meeting about avoiding putting channel metadata into bundle entities. That's definitely out of scope for this milestone though, so I don't want to distract from the task at hand.
| BeforeEach(func() { | ||
| By("initializing cluster state") | ||
| pkgName = "prometheus" | ||
| pkgVer = "0.47.0" |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
It's easy enough to copy/paste the test w/o the package version.
| 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))) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Yeah, I though the message ought to include the channel as well, that might require updating other tests.
There was a problem hiding this comment.
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.
Signed-off-by: Todd Short <[email protected]>
Signed-off-by: Todd Short <[email protected]>
Signed-off-by: Todd Short <[email protected]>
| 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")) |
There was a problem hiding this comment.
For the success cases, can we also fetch the expected Bundle Deployment and check the .spec.source.image.ref?
Signed-off-by: Todd Short <[email protected]>
|
The channel name regex is now this: Which out to allow for the channel names, but might be too flexible? |
I think this fine to start with. We'll probably have to have a few discussions around channels and what they ought to look like in v1. Looking at the tests, I think this is a great start. If we find we need to lock it down further, once we know what we want, we can just revisit this. Really nice PR! Thank you ^^ |
|
@tmshort maybe wait on @joelanford or @grokspawn to sign off before merging? Just in case I've missed something =S |
|
@joelanford @grokspawn you good with this? |
Adds backend support for channelName
Fixes #170
Fixes OPRUN-2917