-
Notifications
You must be signed in to change notification settings - Fork 247
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
Bug 1827723: Make default channel optional #318
Bug 1827723: Make default channel optional #318
Conversation
@Bowenislandsong: This pull request references Bugzilla bug 1827723, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
f28a26a
to
b7bc827
Compare
/hold writing a test to prove bundles with blank default is not breaking registry. |
/hold cancel |
92f4097
to
167568c
Compare
the new annotation for prometheus 0.14.0 in the e2e test:
|
switched channels and default channel situation for 0.15.0 case so that this bundle is added in the middle between 0.14.0 and 0.22.2 |
167568c
to
890caf6
Compare
/retest |
890caf6
to
95f23b2
Compare
/retest |
@@ -7,7 +7,7 @@ metadata: | |||
annotations: | |||
alm-examples: '[{"apiVersion":"monitoring.coreos.com/v1","kind":"Prometheus","metadata":{"name":"example","labels":{"prometheus":"k8s"}},"spec":{"replicas":2,"version":"v2.3.2","serviceAccountName":"prometheus-k8s","securityContext": {}, "serviceMonitorSelector":{"matchExpressions":[{"key":"k8s-app","operator":"Exists"}]},"ruleSelector":{"matchLabels":{"role":"prometheus-rulefiles","prometheus":"k8s"}},"alerting":{"alertmanagers":[{"namespace":"monitoring","name":"alertmanager-main","port":"web"}]}}},{"apiVersion":"monitoring.coreos.com/v1","kind":"ServiceMonitor","metadata":{"name":"example","labels":{"k8s-app":"prometheus"}},"spec":{"selector":{"matchLabels":{"k8s-app":"prometheus"}},"endpoints":[{"port":"web","interval":"30s"}]}},{"apiVersion":"monitoring.coreos.com/v1","kind":"Alertmanager","metadata":{"name":"alertmanager-main"},"spec":{"replicas":3, "securityContext": {}}}]' | |||
spec: | |||
replaces: prometheusoperator.0.15.0 | |||
replaces: prometheusoperator.0.14.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we reordering these? I would prefer that, if we need manifests for a specific test, that we make a copy of them and place them in a separate test folder rather than modify our default examples.
pkg/lib/bundle/generate.go
Outdated
@@ -12,6 +12,7 @@ import ( | |||
"gopkg.in/yaml.v2" | |||
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" | |||
k8syaml "k8s.io/apimachinery/pkg/util/yaml" | |||
"k8s.io/kubectl/pkg/util/slice" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this external dependency really necessary? In general, I think that utils like this which are not included in the standard library and are trivial to write should just be implemented directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, i see your point, just loved that library. getting rid of it.
pkg/lib/bundle/generate.go
Outdated
@@ -298,6 +302,10 @@ func ValidateChannelDefault(channels, channelDefault string) (string, error) { | |||
var chanErr error | |||
channelList := strings.Split(channels, ",") | |||
|
|||
if len(channelList) == 0 || slice.ContainsString(channelList, "", nil) { | |||
return chanDefault, fmt.Errorf("invalid channels is provied: %s", channels) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This error message seems confusing. Could we be more descriptive of what is actually going wrong here? It seems like we are actually checking two things that should have separate error messages:
- That no channels were specified
- That the list of channels included an empty string in the list I think?
Also spelling/grammar nit: use are
instead of is
for plurals, and provided
instead of provied
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ha, I copied that misspell and didn't even see it. Is it weird that I found this to be amusing?
operator-registry/pkg/lib/bundle/generate.go
Line 319 in a25687c
return chanDefault, fmt.Errorf("Invalid channels is provied: %s", channels) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://play.golang.org/p/uemuPhlTisX found out that this split will always output an array with at least 1 length. My bad.
test/e2e/opm_test.go
Outdated
@@ -75,7 +73,7 @@ func buildBundlesWith(containerTool string) error { | |||
bundleTag3: bundlePath3, | |||
} { | |||
if err := inTemporaryBuildContext(func() error { | |||
return bundle.BuildFunc(path, "", bundleImage+":"+tag, containerTool, packageName, channels, defaultChannel, false) | |||
return bundle.BuildFunc(path, "", bundleImage+":"+tag, containerTool, "", "", "", false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment here. Can we have a separate test for this rather than modifying an existing one?
b7f3034
to
50dbba0
Compare
Looking good. |
Thanks, Kevin suggested adding a separate thing for e2e test that I am interested in and not changing the existing package for my testing purpose. |
@@ -0,0 +1,34 @@ | |||
apiVersion: apiextensions.k8s.io/v1beta1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than add something to the root of the manifests/
folder, can we just add these to a test_data
folder for a separate test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awww, I changed all those unit tests for it. Shouda thought about it... making the change now
This commit address the bug where a bundle was released before the current default channel was created, the inference fails. The fix checks if the default channel is included in channels.
This commit leaves default channel blank if not specified or is not contained in channels. This affects the `annotations.yaml` and make it "" for default channels. This will not affect index.db as the code to choose a channel to be the default still lives there. It should remain there since OLM uses that value.
50dbba0
to
689410c
Compare
/lgtm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Bowenislandsong, dinhxuanvu The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@Bowenislandsong: All pull requests linked via external trackers have merged: operator-framework/operator-registry#318. Bugzilla bug 1827723 has been moved to the MODIFIED state. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
This commit address the bug where a bundle was released before the current default channel was created, the inference fails.