Conversation
Signed-off-by: Pavel Okhlopkov <pavel.okhlopkov@flant.com>
Signed-off-by: Pavel Okhlopkov <pavel.okhlopkov@flant.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
What this PR does / why we need it
This PR implements shell-operator refactoring. None of the changes alter runtime behaviour; they are all internal decoupling improvements that reduce coupling to global
app.*state, remove shared mutable globals that block test isolation, and tighten a previously-silent failure path.1.1 — Replace
app.*reads in business logic with injected configurationFiles:
pkg/hook/hook_manager.go,pkg/shell-operator/kube_client.go,pkg/shell-operator/bootstrap.go,test/hook/context/generator.gohook_manager.go— Threeapp.*reads (app.DebugKeepTmpFiles,app.LogProxyHookJSON,app.ProxyJsonLogKey) are replaced with fields stored onManager(keepTemporaryHookFiles,logProxyHookJSON,logProxyHookJSONKey). The values are supplied through three newManagerConfigfields. Bootstrap sets them fromapp.*at the composition root; thepkg/appimport is removed fromhook_manager.go.kube_client.go— IntroducesKubeClientConfigstruct with explicit fields (Context,Config,QPS,Burst,Timeout).defaultMainKubeClientanddefaultObjectPatcherKubeClientnow accept aKubeClientConfiginstead of reaching intoapp.*directly. Theinit*wrapper functions remain the only point that readsapp.*, preserving clean separation between the composition root and infrastructure code.WithTimeoutis now guarded withcfg.Timeout > 0to avoid setting an unnecessary zero timeout on the main client.test/hook/context/generator.go— Removes thepkg/appimport; passes explicit zero/false values tohook.NewHook.1.2 — Remove
DefaultFactoryStoreglobal; inject via constructorFiles:
pkg/kube_events_manager/factory.go,pkg/kube_events_manager/resource_informer.go,pkg/kube_events_manager/monitor.go,pkg/kube_events_manager/kube_events_manager.go,pkg/kube_events_manager/monitor_test.go,test/hook/context/generator.goThe
DefaultFactoryStorepackage-level variable and itsinit()initialiser are removed. Instead:NewKubeEventsManagercreates its own*FactoryStoreper instance.monitorreceives the store via its constructor (NewMonitor) and threads it throughCreateInformersForNamespace→resourceInformerConfig.resourceInformerstores and usesei.factoryStoreinstart()andwait()instead of the global.Each test that creates a
KubeEventsManageror aMonitornow gets an isolatedFactoryStore, eliminating the shared-state race that could cause non-deterministic failures in parallel test runs. The manualDefaultFactoryStore.Reset()call intest/hook/context/generator.gois also removed as it is no longer needed.1.3 — Merge
DebugKeepTmpFilesVar(string) intoDebugKeepTmpFiles(bool)Files:
pkg/app/debug.go,pkg/hook/hook.goTwo variables represented the same concept:
DebugKeepTmpFilesVar string(CLI flag sink, compared via!= "yes") andDebugKeepTmpFiles bool(used at hook construction time). The string variable is removed; the CLI flag is re-wired directly toBoolVar(&DebugKeepTmpFiles). Theapp.DebugKeepTmpFilesVar != "yes"guard inhook.Run()is replaced with theh.KeepTemporaryHookFilesbool field that was already set at construction time, allowing thepkg/appimport to be dropped frompkg/hook/hook.go.1.4 — Fix
HookMetadataAccessorto return(HookMetadata, bool)Files:
pkg/hook/task_metadata/task_metadata.go,pkg/shell-operator/operator.go,pkg/shell-operator/operator_test.go,pkg/hook/task_metadata/task_metadata_test.go,pkg/task/queue/task_queue_compaction_test.goHookMetadataAccessorpreviously returned a zeroHookMetadata{}on both the nil-metadata and wrong-type-assertion paths, while only logging an error. Callers would continue with empty hook names and silently dispatch no-op work.The function signature changes to
(HookMetadata, bool). All three call sites inoperator.go(taskHandler,taskHandleEnableKubernetesBindings,taskHandleHookRun) now explicitly short-circuit withStatus: "Fail"whenok == false, preventing silent data corruption.1.5 — Rename
ManagerConfigabbreviated fields (Kmgr,Smgr,Wmgr,Cmgr)Files:
pkg/hook/hook_manager.go,pkg/shell-operator/bootstrap.go,pkg/hook/hook_manager_test.goThe four fields in
ManagerConfigused non-self-documenting abbreviations. They are renamed to match the types they carry:KmgrKubeEventsManagerSmgrScheduleManagerWmgrAdmissionWebhookManagerCmgrConversionWebhookManagerAll construction sites updated accordingly.