refactor(cmds): adds modular command for datastore#709
Conversation
🦋 Changeset detectedLatest commit: d647973 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 |
4290966 to
0841d05
Compare
There was a problem hiding this comment.
Pull request overview
This PR refactors the datastore command implementation to use a modular architecture, moving from a monolithic structure in the legacy CLI to a new modular package design.
Changes:
- Extracts datastore commands into a new modular
engine/cld/commands/datastorepackage - Implements dependency injection pattern for better testability
- Updates the legacy CLI wrapper to delegate to the new modular implementation
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| engine/cld/commands/datastore/command.go | Defines the main command structure with configuration and validation |
| engine/cld/commands/datastore/deps.go | Implements dependency injection pattern for datastore operations |
| engine/cld/commands/datastore/merge.go | Implements the merge subcommand with flag handling |
| engine/cld/commands/datastore/sync.go | Implements the sync-to-catalog subcommand |
| engine/cld/commands/datastore/command_test.go | Comprehensive test coverage for all command functionality |
| engine/cld/legacy/cli/commands/datastore.go | Updated to delegate to the new modular package |
| engine/cld/legacy/cli/commands/datastore_test.go | Updated tests to reflect new command structure |
| engine/cld/commands/commands.go | Adds Datastore method to Commands struct |
| .changeset/purple-grapes-shout.md | Changeset documentation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
0841d05 to
dbffca4
Compare
| Domain: dom, | ||
| }) | ||
| if err != nil { | ||
| return &cobra.Command{ |
There was a problem hiding this comment.
the previous behaviour was to panic if there is an error, should we stick with that ?
There was a problem hiding this comment.
Ye it was for a moment but I resigned from the panic. Looked strange there.
| f := syncToCatalogFlags{ | ||
| environment: flags.MustString(cmd.Flags().GetString("environment")), | ||
| } | ||
|
|
There was a problem hiding this comment.
i am starting to think maybe it is not even worth using a flag struct eg syncToCatalogFlags to pass the env in.
Reasons:
- runSyncToCatalog knows about cobra anyway from the first argument
cobra.Command, it might as well also fetch the flag value itself - only single flag value in this case, and if we perform fetch inside
runSyncToCatalog, then code can just me
RunE: func(cmd *cobra.Command, _ []string) error {
return runSyncToCatalog(cmd, cfg)
What do you think @jkongie
There was a problem hiding this comment.
When we have 1 flag it seems nice but in case where we have 3 flags struct seems to be more easy to understand at glance approach in my opinion. Where in mcms you got like 5+ flags 😅
There was a problem hiding this comment.
yeah well not fuss, i approved it anyway :D
dbffca4 to
27f47d2
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
27f47d2 to
d647973
Compare
|
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## chainlink-deployments-framework@0.80.0 ### Minor Changes - [#713](#713) [`643bbb7`](643bbb7) Thanks [@bytesizedroll](https://github.com/bytesizedroll)! - refactor: update stale migration terminology to changeset Replace legacy "migration" terminology with "changeset" throughout comments, variable names, and error messages for consistency with durable pipelines. Changes include: - Rename function params: loadMigration → loadChangesets - Rename variables: migDirPath → dirPath, migration → registry - Rename test mocks: mockMigrationDS → mockSourceDS - Update doc comments and error messages - Remove dead commented-out migration test code ### Patch Changes - [#707](#707) [`52eb9f5`](52eb9f5) Thanks [@patricios-space](https://github.com/patricios-space)! - bump chainlink-ton and mcms to bring latest version that uses new exit code interface - [#705](#705) [`50640db`](50640db) Thanks [@bytesizedroll](https://github.com/bytesizedroll)! - remove migration archive functionality - [#709](#709) [`461acc5`](461acc5) Thanks [@ajaskolski](https://github.com/ajaskolski)! - refactor: adds modular cmd for datastore - [#715](#715) [`124dfef`](124dfef) Thanks [@gustavogama-cll](https://github.com/gustavogama-cll)! - chore: add ton to mcms chain access adapter - [#712](#712) [`3584647`](3584647) Thanks [@ecPablo](https://github.com/ecPablo)! - reduce log level of skipping chains logs from WARN to INFO - [#710](#710) [`3c33586`](3c33586) Thanks [@ecPablo](https://github.com/ecPablo)! - fix nil values handling during upf yaml marshalling - [#708](#708) [`d357cb6`](d357cb6) Thanks [@gustavogama-cll](https://github.com/gustavogama-cll)! - fix: run health check and try multiple RPCs in fork tests - [#711](#711) [`a72317e`](a72317e) Thanks [@bytesizedroll](https://github.com/bytesizedroll)! - Rename all "Migration" terminology in domain layer methods to "Changeset" for consistency with durable pipelines terminology. EnvDir: - MergeMigrationDataStore -> MergeChangesetDataStore - MergeMigrationDataStoreCatalog -> MergeChangesetDataStoreCatalog - MergeMigrationAddressBook -> MergeChangesetAddressBook - RemoveMigrationAddressBook -> RemoveChangesetAddressBook ArtifactsDir: - MigrationDirPath -> ChangesetDirPath - CreateMigrationDir -> CreateChangesetDir - RemoveMigrationDir -> RemoveChangesetDir - MigrationDirExists -> ChangesetDirExists - MigrationOperationsReportsFileExists -> ChangesetOperationsReportsFileExists - LoadAddressBookByMigrationKey -> LoadAddressBookByChangesetKey - LoadDataStoreByMigrationKey -> LoadDataStoreByChangesetKey Internal helpers: - loadDataStoreByMigrationKey -> loadDataStoreByChangesetKey - loadAddressBookByMigrationKey -> loadAddressBookByChangesetKey Parameter renames: migKey/migkey -> csKey BREAKING CHANGE: All public domain layer methods with "Migration" in their name have been renamed to use "Changeset" instead. Update all callers to use the new method names. --------- Co-authored-by: app-token-issuer-engops[bot] <144731339+app-token-issuer-engops[bot]@users.noreply.github.com>




Adds Datastore cmd to modular format.
Backwards compatible.
https://smartcontract-it.atlassian.net/browse/CLD-1157