✨ Cleaner Condition Types & Reasons

[bentito]

Description

Fixes: #996

Cleaner Condition Types & Reasons. See commit messages for coverage of this PR and its sync to the RFC recommendations. https://docs.google.com/document/d/1JWJxnDXM0X1JQ67ZIDx5j1ayb1d7ajrK_uSDDaz0G98

Reviewer Checklist

• API Go Documentation
• Tests: Unit Tests (and E2E Tests, if appropriate)
• Comprehensive Commit Messages
• Links to related GitHub Issue(s)

[netlify]

✅ Deploy Preview for olmv1 ready!

Name	Link
🔨 Latest commit	f678a7c
🔍 Latest deploy log	https://app.netlify.com/sites/olmv1/deploys/669e4e46518dfc0008abd4fa
😎 Deploy Preview	https://deploy-preview-1007--olmv1.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[codecov]

Codecov Report

Attention: Patch coverage is 50.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 72.93%. Comparing base (a0fca0d) to head (f678a7c).

Files	Patch %	Lines
...nternal/controllers/clusterextension_controller.go	50.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1007      +/-   ##
==========================================
+ Coverage   72.75%   72.93%   +0.17%     
==========================================
  Files          31       31              
  Lines        1883     1862      -21     
==========================================
- Hits         1370     1358      -12     
+ Misses        375      366       -9     
  Partials      138      138              

Flag	Coverage Δ
e2e	56.68% <0.00%> (+0.32%)	⬆️
unit	44.41% <50.00%> (-0.15%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[bentito]

NB: go-apidiff is an expected failing test as we're intentionally removing API items pre-GA.

[joelanford]

Installed=False is not always correct here. If we have previously installed a bundle, and now the unpack of the next bundle is pending, we still have an installed bundle.

The installation status and the bundle unpack status are not related to each other.

[bentito]

okay, just setting the unpack as failed in 0306c9d

[joelanford]

Unpack state is pending, but we're setting Unpacked=False, Failed?

[bentito]

Well this was changed to acknowledge that we've switched to direct image registry and that as far as opr-ctrl is concerned we can't really be in an Unknown state, we're either failed or not. Perhaps we should just not process StatePending here?

[joelanford]

I'm not sure we should write this code with specific knowledge of the underlying Source implementation. In theory, the source could be switched out to a different implementation that can return StatePending. I've been tinkering with an async direct image client, so this is not entirely hypothetical.

More broadly, I wonder if we even need an Unpacked condition type? I don't see it in the diagram in the RFC. The only reason we are unpacking a bundle is so that we can install it. So can we rollup unpacking status into Progressing and/or Installed conditions?

[bentito]

That's farther than the RFC went, but I think it's in the same spirit. I think we can roll it into TypeInstalled. I think TypeProgressing would actually be adding a new type?

[bentito]

as noted above there is no TypeProgressing currently, if we want to eliminate TypeUnpacked in favor of TypeInstalled
@joelanford, these two helper methods encapsulate all the places changes would be needed:

func setStatusUnpackFailed(ext *ocv1alpha1.ClusterExtension, message string) {
	ext.Status.InstalledBundle = nil
	apimeta.SetStatusCondition(&ext.Status.Conditions, metav1.Condition{
		Type:               ocv1alpha1.TypeUnpacked,
		Status:             metav1.ConditionFalse,
		Reason:             ocv1alpha1.ReasonUnpackFailed,
		Message:            message,
		ObservedGeneration: ext.GetGeneration(),
	})
}

func setStatusUnpacked(ext *ocv1alpha1.ClusterExtension, message string) {
	apimeta.SetStatusCondition(&ext.Status.Conditions, metav1.Condition{
		Type:               ocv1alpha1.TypeUnpacked,
		Status:             metav1.ConditionTrue,
		Reason:             ocv1alpha1.ReasonUnpackSuccess,
		Message:            message,
		ObservedGeneration: ext.GetGeneration(),
	})
}

So on the fail case we'd want TypeInstalled as ConditionUnknown and same on the success case. The reason would be the difference. But I wonder if the success case is a bit odd: TypeInstalled is Uknown and the reason is Unpack success?

[joelanford]

I think TypeInstalled should literally just be based on: "is there a helm release secret with status deployed? if so, Installed=True. If not "Installed=False". If there was an error looking up helm release secrets (other than "not found"), then "Installed=Unknown".

Nothing else should play into it.

[bentito]

I created #1027 to capture this need. I'd like to keep this PR limited to the RFC covered items.

[tmshort]

@joelanford are you ok with this?

[tmshort]

/lgtm