Enhance CLI by adding instance management#364
Merged
ArangoGutierrez merged 1 commit intoNVIDIA:mainfrom May 23, 2025
Merged
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the Holodeck CLI by adding instance management commands and refactoring existing ones to use a centralized instance manager.
- Introduces new
listandstatuscommands for inspecting instances. - Refactors
createanddeleteto generate unique IDs, manage cache paths dynamically, and improve logging. - Improves CLI UX with richer help text, examples, and Bash completion.
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/data/test_aws.yml | Updated test fixture paths and added an IP allow rule. |
| internal/instances/instances.go | Added Manager implementation for instance lifecycle. |
| internal/instances/instances_test.go | New unit tests for the instance manager. |
| cmd/cli/list/list.go | New list command for tabular instance display. |
| cmd/cli/status/status.go | New status command for detailed instance info. |
| cmd/cli/delete/delete.go | Refactored delete to support env file or instance ID. |
| cmd/cli/create/create.go | Refactored create to assign instance IDs dynamically. |
| cmd/cli/main.go | Enhanced global help, description, examples, and flags. |
Comments suppressed due to low confidence (4)
cmd/cli/delete/delete.go:61
- [nitpick] The flag name "envFile" is inconsistent with typical CLI conventions (use lowercase and hyphens); consider renaming to "env-file" for consistency.
Name: "envFile",
cmd/cli/list/list.go:64
- There are no unit tests for the
listcommand; consider adding tests to verify header output, no-instances behavior, and proper formatting.
func (m command) run(c *cli.Context) error {
cmd/cli/status/status.go:66
- The
statuscommand lacks automated tests; consider adding tests for required-argument validation, successful lookup, and fallback to GetInstanceByFilename.
func (m command) run(c *cli.Context, instanceID string) error {
cmd/cli/main.go:66
- [nitpick] The global examples show
holodeck delete <instance-id>but the command actually requires-i <instance-id>; consider updating the example to include the flag for clarity.
# Delete an environment
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the CLI for managing Holodeck instances by introducing new instance management commands (list and status), improving create and delete functionality, and refining the overall user experience.
- New commands for listing and checking the status of instances
- Updates to the create and delete commands for dynamic cache handling and improved logging
- Enhanced CLI help text with custom templates and Bash completion
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/data/test_aws.yml | Updated AWS test configuration (privateKey path, additional CIDR rule) |
| internal/instances/instances_test.go | Added tests for instance manager functions |
| internal/instances/instances.go | Implemented instance management functions and status querying |
| cmd/cli/status/status.go | Added status command to display detailed instance information |
| cmd/cli/main.go | Updated CLI main entry with new commands and help template |
| cmd/cli/list/list.go | Introduced list command for tabular display of instances |
| cmd/cli/delete/delete.go | Refactored delete command to support removal by env file or instance ID |
| cmd/cli/create/create.go | Improved create command with dynamic instance ID generation and cache handling |
Comments suppressed due to low confidence (1)
cmd/cli/create/create.go:119
- [nitpick] The variable opts.cachePath is later overwritten with a file path, which may be confusing since it was initially used as the cache directory. Consider introducing a separate variable (e.g., instanceCacheFile) for the file path to improve clarity.
instanceID := manager.GenerateInstanceID()
There was a problem hiding this comment.
Pull Request Overview
This PR improves the Holodeck CLI by enhancing instance management functionality. The key changes include:
- Introducing two new commands: "list" for displaying all instances and "status" for detailed information of a single instance.
- Enhancing the "create" and "delete" commands with dynamic cache file handling and instance ID management.
- Updating the CLI’s overall help text and enabling Bash completion.
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/instances/instances_test.go | Added tests for instance manager functions |
| internal/instances/instances.go | Implements instance operations and status checks |
| cmd/cli/status/status.go | Introduces the "status" command with detailed instance info |
| cmd/cli/main.go | Updates main CLI setup with new commands and a custom help template |
| cmd/cli/list/list.go | Implements the "list" command for displaying instances |
| cmd/cli/delete/delete.go | Refactors the "delete" command to support deletion by env file or instance ID |
| cmd/cli/create/create.go | Enhances the "create" command by generating unique instance IDs and dynamic cache file management |
e7d46ce to
dcab961
Compare
Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>
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.
This pull request introduces significant updates to the CLI for managing Holodeck instances. Key changes include the addition of new commands (
listandstatus), enhancements to thecreateanddeletecommands, and improvements to the overall CLI user experience. The changes aim to streamline instance management and provide more detailed information about instances.❯ ./bin/holodeck create -f tests/test_aws.yml ✔ Creating VPC ✔ Creating subnet ✔ Creating Internet Gateway ✔ Creating route table ✔ Creating security group ✔ Creating EC2 instance Created instance 84e1dca0 ❯ ./bin/holodeck list INSTANCE ID NAME PROVIDER STATUS CREATED AGE 84e1dca0 holodeck-aws-e2e-test aws running 2025-05-21 19:01:30 1m48s ❯ ./bin/holodeck status 84e1dca0 Instance ID: 84e1dca0 Name: holodeck-aws-e2e-test Provider: aws Status: running Created: 2025-05-21 19:01:30 (2m2s ago) Cache File: /Users/eduardoa/.cache/holodeck/84e1dca0.yaml ❯ ./bin/holodeck delete -i 84e1dca0 ✔ Waiting for instance i-0e28582e748d1ff59 to be terminated ✔ Deleting VPC resources Successfully deleted instance 84e1dca0 (holodeck-aws-e2e-test)New Commands
listCommand: Added a newlistcommand to display all Holodeck instances in a tabular format, including details like instance ID, name, provider, status, creation time, and age.statusCommand: Added a newstatuscommand to show detailed information about a specific Holodeck instance, such as its ID, name, provider, status, creation time, and cache file.Enhancements to Existing Commands
createCommand:deleteCommand:CLI Improvements