Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Merged
evgeniy-scherbina merged 1 commit intomainfromyevhenii/510-reconciliation-loop
Apr 29, 2025

Conversation

evgeniy-scherbina
Copy link
Contributor

No description provided.

@@ -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() {
Copy link
ContributorAuthor

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?

Copy link
Contributor

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.

Copy link
ContributorAuthor

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

Copy link
ContributorAuthor

@evgeniy-scherbinaevgeniy-scherbinaApr 29, 2025
edited
Loading

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?

Copy link
Contributor

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 👍

@evgeniy-scherbinaevgeniy-scherbina marked this pull request as ready for reviewApril 28, 2025 20:51
@evgeniy-scherbinaevgeniy-scherbina changed the titlerefactor: skipping reconciliation for some of the presetsrefactor: skip reconciliation for some presetsApr 28, 2025
@evgeniy-scherbinaevgeniy-scherbina merged commit02b2de9 intomainApr 29, 2025
35 checks passed
@evgeniy-scherbinaevgeniy-scherbina deleted the yevhenii/510-reconciliation-loop branchApril 29, 2025 11:55
@github-actionsgithub-actionsbot locked and limited conversation to collaboratorsApr 29, 2025
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@dannykoppingdannykoppingdannykopping approved these changes

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

2 participants
@evgeniy-scherbina@dannykopping

[8]ページ先頭

©2009-2025 Movatter.jp