Merged
Conversation
bdd07c8 to
fd8c50e
Compare
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
Followed: #862
Implements architecture refactoring roadmap (
review.md): seven focused, medium-risk improvements covering a data race, a deferred context, two OCP violations, three interface splits, and two structural decompositions. Build is clean; all 26 test packages pass; 5 new test files added (709 insertions, 218 deletions across 16 files).What this PR does / why we need it
2.1 — Extract
HookTaskFactory(operator.go, newhook_task_factory.go)initHookManager,initValidatingWebhookManager, andconversionEventHandlereach contained an identical anonymous callback building atask.NewTask(HookRun).WithMetadata(HookMetadata{...}).WithLogLabels(...).WithQueueName(...).WithCompactionID(...)chain (~15 duplicated lines, three times). Extracted toHookTaskFactory.NewHookTask(hook, bindingType, info, logLabels). The factory is a zero-value struct field onShellOperator; no constructor wiring required.2.2 — Decompose
loadHook()into three discrete methods (hook_manager.go)loadHook()was ~80 lines performing path resolution,--configexecution, config parsing, metadata label injection for four binding types, controller wiring, and validation. Refactored into:fetchHookConfig(hook)--config, parse output, populatehook.ConfigenrichHookMetadata(hook)hook.Namelog/metric labels into all four binding type configswireHookController(hook)HookController, init all binding subsystems, setTmpDirloadHook()now orchestrates only: construct hook →fetchHookConfig→enrichHookMetadata→wireHookController→ validate → log.2.3 —
IOProviderinterface abstracts temp-file I/O fromHook.Run()(hook.go, newio_provider.go)Hook.Run()previously called five privateprepare*File()helpers that created real OS temp files, making the method impossible to unit-test without a filesystem and manual cleanup. Introduced:FileIOProvider{}is the production implementation (writes totmpDir). Tests can supply any alternative implementation.NewHookinjectsFileIOProvider{}by default; the field is exported so test code can substitute it. The fiveprepare*methods are removed;Run()callsh.IOProvider.PrepareFiles(...)and uses the returnedHookRunFilesstruct for all path references.2.4 —
VersionedConverterregistry; remove versionswitch(config.go)HookConfig.ConvertAndCheckcontained aswitch c.Version { case "v0": ... case "v1": ... }— a textbook OCP violation. Introduced theVersionedConverterinterface (one method:ConvertAndCheck(data []byte, c *HookConfig) error) and avar versionedConverters = map[string]VersionedConverter{ "v0": v0Converter{}, "v1": v1Converter{} }registry. The switch is replaced by a single map lookup. New versions are added by registering a new struct — no changes toConvertAndCheckneeded.2.5 — Split
KubeEventsManagerandScheduleManagerinterfacesBoth interfaces previously mixed lifecycle control, entry/monitor management, and channel emission into a single surface.
KubeEventsManager→MonitorRegistry(CRUD:AddMonitor,HasMonitor,GetMonitor,StartMonitor,StopMonitor,WithMetricStorage,MetricStorage) +EventEmitter(Ch,Stop,Wait).KubeEventsManagercomposes both.ScheduleManager→ScheduleRegistry(Add,Remove) +ScheduleEmitter(Ch,Start,Stop).ScheduleManagercomposes both.Callers that only need event consumption can now depend on
EventEmitter/ScheduleEmitter; callers that only register watches depend onMonitorRegistry/ScheduleRegistry. The concrete types satisfy all sub-interfaces (verified by compile-timevar _ X = (*impl)(nil)assertions in tests).2.6 — Propagate
context.Contextthroughbootstrap.Init()(bootstrap.go,start.go)Init()previously accepted no context and calledcontext.TODO()in six places — fourlogger.Logcalls,NewShellOperator, andRunDefaultDebugServer. The function signature is nowInit(ctx context.Context, logger *log.Logger)and every internal call uses the provided context. The single call-site incmd/shell-operator/start.gopasses thecontext.Background()that already existed there.2.7 — Fix
CronEntry.Idsdata race (schedule_manager.go)sm.Entrieswasmap[string]CronEntry(value). MutatingcronEntry.Ids[newEntry.Id]on a copy retrieved from the map silently discarded the write; the stored entry was never updated. Under concurrent access the check-then-modify pair was also racy. Changed tomap[string]*CronEntryso all reads and writes operate on the same heap object under the existing mutex.