- Notifications
You must be signed in to change notification settings - Fork907
refactor: skip reconciliation for some presets#17595
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
@@ -310,6 +310,15 @@ func (c *StoreReconciler) ReconcilePreset(ctx context.Context, ps prebuilds.Pres | |||
return nil | |||
} | |||
// Nothing has to be done. | |||
if !ps.Preset.UsingActiveVersion && actions.IsNoop() { |
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.
@dannykopping Do we need!ps.Preset.UsingActiveVersion
? I think we can skip reconciliation for all presets ifactions.IsNoop() == true
?
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 guess, yeah, but if the preset is part of an inactive version then we shouldn't be logging anything for that; for active presets which have no operations to perform, that's at least diagnostically useful.
Consider an installation that's been using prebuilds for a while and they have >100 inactive preset versions; that'll log a tonne for no value.
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.
okay, let's keep it as it is
evgeniy-scherbinaApr 29, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 mean here we log withdebug
level which is probably okay?
// Nothing has to be done.if!ps.Preset.UsingActiveVersion&&actions.IsNoop() {logger.Debug(ctx,"skipping reconciliation for preset - nothing has to be done",slog.F("template_id",ps.Preset.TemplateID.String()),slog.F("template_name",ps.Preset.TemplateName),slog.F("template_version_id",ps.Preset.TemplateVersionID.String()),slog.F("template_version_name",ps.Preset.TemplateVersionName),slog.F("preset_id",ps.Preset.ID.String()),slog.F("preset_name",ps.Preset.Name))returnnil}
and then exit early without additional logs withinfo/warn
log level
Or you wanted to delete debug log as well?
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.
Nope we can keep the debug log 👍
02b2de9
intomainUh oh!
There was an error while loading.Please reload this page.
No description provided.