feat: populate Config attribute in hook params#924
feat: populate Config attribute in hook params#924gustavogama-cll wants to merge 1 commit intomainfrom
Conversation
|
There was a problem hiding this comment.
Pull request overview
Adds changeset configuration (Config) to hook parameter structs and proposal metadata so post-proposal hooks can receive the same config that was used when generating/applying a changeset.
Changes:
- Populate
Configon pre/post hooks duringChangesetsRegistry.Applyand on post-proposal hooks viaRunProposalHooks. - Persist changeset
configinto MCMS timelock proposal metadata and plumb it through themcms hookscommand. - Add/adjust tests to assert config propagation and handle large numeric values.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| engine/cld/commands/pipeline/run.go | Writes changeset config into proposal metadata alongside input. |
| engine/cld/commands/pipeline/run_test.go | Updates expectations for proposal metadata (config) and large numeric input. |
| engine/cld/commands/mcms/cmd_run_hooks.go | Decodes config from proposal metadata and passes it into proposal hooks. |
| engine/cld/commands/mcms/cmd_run_hooks_test.go | Validates proposal hooks receive/log the decoded config. |
| engine/cld/changeset/registry.go | Populates hook params with Config and adds GetConfiguration. |
| engine/cld/changeset/registry_test.go | Adds coverage for config propagation in hooks and GetConfiguration. |
| engine/cld/changeset/postprocess.go | Ensures post-processed changesets forward configuration(). |
| engine/cld/changeset/postprocess_test.go | Tests configuration() forwarding through post-processing wrapper. |
| engine/cld/changeset/mcms.go | Extends RunProposalHooks to accept and pass through config. |
| engine/cld/changeset/mcms_test.go | Updates tests for new RunProposalHooks signature + config param. |
| engine/cld/changeset/hooks.go | Adds Config field to PostProposalHookParams. |
| engine/cld/changeset/common.go | Adds configuration() to internal changeset interface and impl. |
| engine/cld/changeset/common_test.go | Tests configuration() behavior across config wiring modes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| changesetConfig, err := applySnapshot.registryEntry.changeset.configuration() | ||
| if err != nil { | ||
| return fdeployment.ChangesetOutput{}, fmt.Errorf("failed to get changeset configuration: %w", err) | ||
| } |
There was a problem hiding this comment.
Apply() now computes changesetConfig via changeset.configuration() before running hooks, but this ignores the WithInput(...) ApplyOption. When cfg.inputStr is set, the changeset execution uses applyWithInput(e, cfg.inputStr) (potentially using configProviderWithInput), while hooks will receive a config derived from the default provider (often DURABLE_PIPELINE_INPUT), so hook params can diverge from the actual config used for the apply. Consider deriving the hook Config from the same source as applyWithInput (e.g., add a configurationWithInput(inputStr string) method to internalChangeSet, or pass cfg.inputStr into configuration() and have ChangeSetImpl use configProviderWithInput when inputStr is non-empty).
| func (r *ChangesetsRegistry) GetConfiguration(key string) (any, error) { | ||
| entry, ok := r.entries[key] | ||
| if !ok { | ||
| return ChangesetConfig{}, fmt.Errorf("changeset '%s' not found", key) |
There was a problem hiding this comment.
GetConfiguration returns ChangesetConfig{} when the key is not found, but the method’s return type is any and callers likely treat the value as the changeset’s configuration (which can be nil). Returning a ChangesetConfig struct here is misleading and inconsistent with the API intent; return nil (or a typed zero of the configuration type if you want a non-nil sentinel) alongside the error instead.
| return ChangesetConfig{}, fmt.Errorf("changeset '%s' not found", key) | |
| return nil, fmt.Errorf("changeset '%s' not found", key) |
| @@ -221,13 +229,15 @@ func saveChangesetProposalMetadata(changesetName string, out fdeployment.Changes | |||
| } | |||
|
|
|||
| proposal.Metadata["changesets"] = []struct { | |||
| ID string `json:"id"` | |||
| Name string `json:"name"` | |||
| Input json.RawMessage `json:"input"` | |||
| ID string `json:"id"` | |||
| Name string `json:"name"` | |||
| Input json.RawMessage `json:"input"` | |||
| Config any `json:"config"` | |||
| }{{ | |||
| ID: id, | |||
| Name: changesetName, | |||
| Input: json.RawMessage(changesetInputJSON), | |||
| ID: id, | |||
| Name: changesetName, | |||
| Input: json.RawMessage(changesetInputJSON), | |||
| Config: changesetConfig, | |||
| }} | |||
There was a problem hiding this comment.
changesetConfig is stored into proposal metadata as any. If a changeset was configured via WithEnvInput/WithJSON with an any-typed payload, it can contain json.Number values (your codebase explicitly uses UseNumber() for this). json.Number marshals as a JSON string under json.MarshalIndent (used by jsonutils.WriteFile), so numeric config values will be persisted quoted and will not round-trip as numbers when loading the proposal later. To preserve numeric semantics, consider storing config as json.RawMessage (like input) and generating that raw JSON with a marshaller that emits json.Number as a number token (or normalizing json.Number to json.RawMessage(n.String()) recursively before writing).
No description provided.