[run bundle] Install plan generation and approval for subscription#3838
Conversation
openshift-ci-robot
requested review from
bharathi-tenneti,
estroz,
jmrodri and
joelanford
| withPackageChannel(o.PackageName, o.Channel, o.StartingCSV), | ||
| withCatalogSource(cs.GetName(), o.cfg.Namespace)) | ||
| withCatalogSource(cs.GetName(), o.cfg.Namespace), | ||
| withInstallPlanApproval(v1alpha1.ApprovalManual)) |
There was a problem hiding this comment.
I'm not sure if CI will pass because InstallPlans created by run packagemanifests will not be approved. IIRC we discussed that automatic approval is a bug in run packagemanifests since a version is specified but that version might not be the channel head specified in a Subscription, and setting approval to manual is correct for both the index and configmap CatalogCreator's; we would need to approve all Subscriptions manually with this change.
There was a problem hiding this comment.
After the catalog source is created, the logic should be identical between run packagemanifests and run bundle.
We shouldn't have different implementations of the InstallPlanApprover interface (and we probably don't need that interface at all).
| if approver, ok := o.CatalogCreator.(InstallPlanApprover); ok { | ||
| if err = approver.Approve(ctx, o.PackageName); err != nil { | ||
| if err = approver.Approve(ctx, subscription); err != nil { | ||
| return nil, err | ||
| } | ||
| } |
There was a problem hiding this comment.
Why is the CatalogCreator involved with approving install plans?
It doesn't seem like we need different implementations between run packagemanifests and run bundle. In both cases, we should create the subscription with manual approval and then immediately approve the install plan for that CSV.
|
|
||
| // Approve approves the install plan for a subscription, which will | ||
| // generate a CSV | ||
| func (c IndexImageCatalogCreator) Approve(ctx context.Context, sub *v1alpha1.Subscription) error { |
There was a problem hiding this comment.
Going along with my other comments, I think this function should be moved to the OperatorInstaller struct and renamed to approveInstallPlan.
The way the catalog source gets created is decoupled from he creation of the subscription and install plans, so we don't need separate approval logic.
There was a problem hiding this comment.
Updated the PR to remove the interface and make approveInstallPlan a method on OperatorInstaller struct, which will be common across both run packagemanifests and run bundle
Thanks so much for the feedback @joelanford @estroz, please review new changes
joelanford
left a comment
joelanford
left a comment
There was a problem hiding this comment.
Just a nit and potential follow-up.
/lgtm
| return fmt.Errorf("install plan is not available for the subscription %s: %v", sub.Name, err) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Non-blocking: In one of these install plan functions, do we need to do a sanity check and ensure the install plan is for the CSV that we created in the subscription.
In the case of an upgrade being available, there can be multiple install plans and I think the subscription may reference the next available.
It may not be strictly necessary based in this case since we would need to approve the initial install plan to do the initial install and let OLM get to the point of doing an upgrade, BUT if it's a simple extra sanity check, it may be a good idea to go ahead and add.
There was a problem hiding this comment.
Great idea! I guess we can add the below check:
if ip.Status.Plan != nil {
for _, installPlan := range ip.Status.Plan {
if installPlan.Resource.Kind == "ClusterServiceVersion" {
if installPlan.Resolving != sub.Status.CurrentCSV {
return fmt.Errorf("install plan CSV doesn't match the CSV in the subscription")
}
}
}
}
I will do this in a follow-up as this is non-blocking.
rashmigottipati
force-pushed
the
install-plan-for-run-bundle
branch
from
06d7d0b to
912d9ce
Compare
5 participants
Description of the change:
Motivation for the change:
/cc @joelanford @jmrodri @estroz @bharathi-tenneti