Add per-changeset hook registration to fluent API#796
Conversation
🦋 Changeset detectedLatest commit: d433e75 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
👋 bytesizedroll, thanks for creating this pull request! To help reviewers, please consider creating future PRs as drafts first. This allows you to self-review and make any final changes before notifying the team. Once you're ready, you can mark it as "Ready for review" to request feedback. Thanks! |
There was a problem hiding this comment.
Pull request overview
Adds per-changeset hook registration to the fluent changeset configuration API, and persists those hooks into the registry layer so they can be executed later as part of the changeset lifecycle.
Changes:
- Add
WithPreHooks(...PreHook)/WithPostHooks(...PostHook)toConfiguredChangeSetandPostProcessingChangeSet. - Store hook slices on
ChangeSetImpl/PostProcessingChangeSetImpland propagate them throughThenWith. - Extract hooks into
registryEntryduringAdd()via an internalhookCarrierinterface.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| engine/cld/changeset/common.go | Adds hook registration methods + stores hooks on ChangeSetImpl and forwards them via ThenWith. |
| engine/cld/changeset/postprocess.go | Converts PostProcessingChangeSet to an interface and adds hook registration + storage on post-processing changesets. |
| engine/cld/changeset/registry.go | Extends registryEntry to include hook slices and extracts them during newRegistryEntry() on Add(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
WithPreHooks, WithPostHooks, and ThenWith so each chained value gets an independent copy, prevents aliasing
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func newRegistryEntry(c ChangeSet, opts ChangesetConfig) registryEntry { | ||
| return registryEntry{changeset: c, options: opts} | ||
| entry := registryEntry{changeset: c, options: opts} | ||
|
|
||
| if hc, ok := c.(hookCarrier); ok { | ||
| entry.preHooks = hc.getPreHooks() | ||
| entry.postHooks = hc.getPostHooks() | ||
| } | ||
|
|
||
| return entry |
There was a problem hiding this comment.
registryEntry now captures preHooks/postHooks via hookCarrier, but those fields are never read anywhere in this package (including ChangesetsRegistry.Apply). As a result, the new WithPreHooks/WithPostHooks fluent API has no observable effect at runtime. Consider either (a) executing these hooks as part of ChangesetsRegistry.Apply (or the underlying changeset Apply) or (b) providing an exported accessor that downstream code can use to retrieve hooks for execution.
| type PostProcessor func(e fdeployment.Environment, config fdeployment.ChangesetOutput) (fdeployment.ChangesetOutput, error) | ||
|
|
||
| type PostProcessingChangeSet internalChangeSet | ||
| type PostProcessingChangeSet interface { |
There was a problem hiding this comment.
nit: thinking about the name of the interface, maybe there's something we could add to also imply the interface handles pre processing too? maybe HookableChangeSet or LifecycleChangeSet.
Summary
WithPreHooks(...PreHook)andWithPostHooks(...PostHook)toConfiguredChangeSetandPostProcessingChangeSetinterfacesChangeSetImplandPostProcessingChangeSetImpl, propagated throughThenWith, and extracted intoregistryEntryvia an internalhookCarrierinterface whenAdd()is calledAdd()call sites and tests are unchangedTest plan
golangci-lint)