-
Notifications
You must be signed in to change notification settings - Fork 19
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
Set default subscription values when not specified #228
Set default subscription values when not specified #228
Conversation
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.
Looking clean overall!
if (subSpec.CatalogSource != "" && subSpec.CatalogSource != catalog) || | ||
(subSpec.CatalogSourceNamespace != "" && subSpec.CatalogSourceNamespace != catalogNamespace) { | ||
return errors.New( | ||
"the subscription defaults could not be determined because the catalog doesn't match those in the " + | ||
"PackageManifest", | ||
) | ||
} |
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.
For a minute, I was thinking that this kind of message could be good to surface regardless of defaulting. But I expect that the subscription would give some kind of error if the catalog doesn't match the package, so the user would still get a message there. Do you think that's right?
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, that sounds right.
"the subscription defaults could not be determined because the catalog doesn't match those in the " + | ||
"PackageManifest", |
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.
"the subscription defaults could not be determined because the catalog doesn't match those in the " + | |
"PackageManifest", | |
"the subscription defaults could not be determined because the catalog specified in the " + | |
"policy does not match what was found in the PackageManifest on the cluster", |
Especially if the user is not familiar with OLM, I think we should give some extra hints on where these values came from.
test/resources/case38_operator_install/operator-policy-all-defaults.yaml
Show resolved
Hide resolved
077037a
to
c35614c
Compare
if err != nil { | ||
if k8serrors.IsNotFound(err) { | ||
return fmt.Errorf( | ||
"%wthe subscription defaults could not be determined because the PackageManifest was not found", |
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.
I don't know if I've seen wrapping a specific empty error like this. It's interesting. I'm still trying to find a way to do this kind of thing that I really like, did you find this somewhere else (just for reference)?
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.
We do this in the propagator with ErrRetryable
but that comes from me, so I don't know of prior art doing this but I think it does the trick. I think the more "right" thing would be to have a custom error type as opposed to wrapping a custom error, but I didn't think it was worth the extra code.
if catalog == "" || catalogNamespace == "" { | ||
return fmt.Errorf( | ||
"%wthe subscription defaults could not be determined because the PackageManifest didn't specify a catalog", | ||
ErrPackageManifest, | ||
) | ||
} |
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.
I'm not sure that this one should wrap the error so it's retried - I think usually this would get fixed by the content in the catalog changing? What was your reasoning
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.
I don't know that this would ever occur, but I figured it was likely due to partial results being returned but it seems unlikely. I'll remove the retry.
if defaultChannel == "" { | ||
return fmt.Errorf( | ||
"%wthe default channel could not be determined because the PackageManifest didn't specify one", | ||
ErrPackageManifest, | ||
) | ||
} |
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 here, I'm not sure if this should be retried.
This sets the default channel, source, and sourceNamespace when not provided in the policy. Relates: https://issues.redhat.com/browse/ACM-10561 Signed-off-by: mprahl <mprahl@users.noreply.github.com>
c35614c
to
8211f51
Compare
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: JustinKuli, mprahl 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 |
e71a482
into
open-cluster-management-io:main
This sets the default channel, source, and sourceNamespace when not provided in the policy.
Relates:
https://issues.redhat.com/browse/ACM-10561