-
Notifications
You must be signed in to change notification settings - Fork 10
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
Add license status to cluster output #312
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.
LGTM, just a couple nits.
@@ -174,10 +175,10 @@ func (r *RedpandaReconciler) Reconcile(c context.Context, req ctrl.Request) (ctr | |||
|
|||
defer func() { | |||
durationMsg := fmt.Sprintf("reconciliation finished in %s", time.Since(start).String()) | |||
log.Info(durationMsg) | |||
log.V(logger.TraceLevel).Info(durationMsg) |
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 should really just be relying on a time series metric for this anyhow 😓
@@ -408,24 +413,13 @@ func (r *RedpandaReconciler) reconcileDefluxed(ctx context.Context, rp *v1alpha2 | |||
return nil | |||
} | |||
|
|||
func (r *RedpandaReconciler) reconcileClusterConfig(ctx context.Context, rp *v1alpha2.Redpanda) error { | |||
func (r *RedpandaReconciler) ratelimitCondition(ctx context.Context, rp *v1alpha2.Redpanda, conditionType string) bool { |
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.
nit: add a comment here noting that this function require the Ready condition to be true.
shouldReconcileCondition
might be a bit more descriptive and ergonomic?
case rpadmin.LicenseStatusValid: | ||
status = metav1.ConditionTrue | ||
reason = "LicenseValid" | ||
message = "Valid" |
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.
Just for future proofing and better debuggability:
default:
reason = "LicenseStatusUnknown"
message = fmt.Sprintf("unknown license status: %q", features.LicenseStatus)
I've been bitten by missing defaults too many times.
…ssertions run too slowly
This adds a cluster license status to the Redpanda CRD. The test to leverage this checks across a couple of different Redpanda versions to show how this behaves with the new auto-installed test license v. no license being present on the system whatsoever.
Edit: note that I also hid more logging behind
TraceLevel
so I can actually see test failures. If these need to be turned up in tests, you can always do so in the test setup temporarily (or maybe we can add a flag to do it on-demand?)