Skip to content
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

fix: fresh objects for Synchronization executions #181

Merged
merged 3 commits into from
Mar 18, 2021

Conversation

diafour
Copy link
Contributor

@diafour diafour commented Mar 12, 2021

Overview

Set objects in "Synchronization" binding context to an actual state in the cluster.

What this PR does / why we need it

See flant/shell-operator#261

Special notes for your reviewer

Does this PR introduce a user-facing change?


@diafour diafour requested review from nabokihms and zuzzas March 12, 2021 11:00
@@ -1398,10 +1444,23 @@ func (op *AddonOperator) HandleGlobalHookRun(t sh_task.Task, labels map[string]s
op.MetricStorage.HistogramObserve("{PREFIX}global_hook_run_seconds", d.Seconds(), metricLabels)
})()

logEntry.Infof("Global hook run")
isSynchronization := hm.BindingType == OnKubernetesEvent && hm.BindingContext[0].Type == types.TypeSynchronization
shouldRunHook := true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move this code to a method for this instead of copying it? And why do we add shouldRunHook by the way? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. I've created a method IsSynchronization.

  2. shouldRunHook is needed as GlobalHookRun and ModuleHookRun tasks are now created for bindings with executeHookOnSynchronization: false. These tasks only enable emitting events from the binding's Monitor and do not run the hook.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

About the second part:
I was wondering about moving some code out of the Handle* methods. If it is hard or gives no benefits, let's leave things as is.

@diafour diafour force-pushed the fix_fresh_objects_on_synchronization_retries branch from d4966f9 to 422f4cf Compare March 17, 2021 15:06
@diafour diafour requested a review from nabokihms March 17, 2021 15:12
Copy link
Member

@nabokihms nabokihms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No serious concerns left. The only thing we have to do is updating go.mod after merging flant/shell-operator#261

@diafour diafour merged commit 65f01a4 into master Mar 18, 2021
@diafour diafour deleted the fix_fresh_objects_on_synchronization_retries branch June 4, 2021 12:20
@diafour diafour added this to the 1.0.0 milestone Aug 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants