Add top-level address-book and datastore CLI commands #683
Conversation
|
|
👋 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
This PR introduces new top-level CLI commands for address book and datastore operations within the engine/cld/legacy/cli/commands package. The changes provide a structured command interface for managing address book artifacts in different deployment environments.
Changes:
- Added
address-bookcommand group with three subcommands:merge,migrate, andremove - Implemented comprehensive test coverage for the new address book commands
- Provided environment-based operations for address book management
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| engine/cld/legacy/cli/commands/addressbook.go | Implements the address-book command structure with merge, migrate, and remove subcommands for managing address book artifacts |
| engine/cld/legacy/cli/commands/addressbook_test.go | Adds comprehensive tests validating command structure, metadata, flags, and documentation for all address-book subcommands |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| err := addressBookCmd.MarkPersistentFlagRequired("environment") | ||
| if err != nil { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Returning nil on error will cause a nil pointer dereference when the caller tries to use the returned command. Consider panicking with a descriptive error message or propagating the error to the caller.
| err := cmd.MarkFlagRequired("name") | ||
| if err != nil { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Returning nil on error will cause a nil pointer dereference when the caller tries to use the returned command. Consider panicking with a descriptive error message or propagating the error to the caller.
| err := cmd.MarkFlagRequired("name") | ||
| if err != nil { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Returning nil on error will cause a nil pointer dereference when the caller tries to use the returned command. Consider panicking with a descriptive error message or propagating the error to the caller.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Give each subtest its own fresh command tree | ||
| root := c.NewDatastoreCmds(dom) | ||
|
|
||
| t.Parallel() | ||
|
|
There was a problem hiding this comment.
The command tree is created before calling t.Parallel(), which could lead to race conditions if multiple subtests access shared state. Move the root creation after t.Parallel() to ensure proper test isolation.
| // Give each subtest its own fresh command tree | |
| root := c.NewDatastoreCmds(dom) | |
| t.Parallel() | |
| t.Parallel() | |
| // Give each subtest its own fresh command tree | |
| root := c.NewDatastoreCmds(dom) |
| // Give each subtest its own fresh command tree | ||
| root := c.NewAddressBookCmds(dom) | ||
|
|
||
| t.Parallel() | ||
|
|
There was a problem hiding this comment.
The command tree is created before calling t.Parallel(), which could lead to race conditions if multiple subtests access shared state. Move the root creation after t.Parallel() to ensure proper test isolation.
| // Give each subtest its own fresh command tree | |
| root := c.NewAddressBookCmds(dom) | |
| t.Parallel() | |
| t.Parallel() | |
| // Give each subtest its own fresh command tree | |
| root := c.NewAddressBookCmds(dom) |
|
1 participant
Summary
Add top-level
address-bookanddatastoreCLI commands, extracting them from the nestedmigrationcommand structure as part of deprecating migrations.New Commands
address-book:merge,migrate,removedatastore:merge,sync-to-catalog