Improve usability of CLI, particularly create command#20
Improve usability of CLI, particularly create command#20jasonrclark wants to merge 10 commits intomainfrom
create command#20Conversation
jasonrclark
changed the title
create command
There was a problem hiding this comment.
Pull request overview
This PR refactors the gh-runtime CLI commands to improve usability and testability, adds unit tests for core commands, and introduces CI to run builds/tests on pushes/PRs. It also updates CLI wording to consistently refer to “app ID” rather than “app name”, and enhances create with additional flags and an --init flow.
Changes:
- Refactor command handlers into testable
run*functions that accept an injected REST client interface, plus add extensive unit tests. - Enhance
createto support creation by--name, optional--visibility, and--initto writeruntime.config.json. - Add GitHub Actions CI workflow to build and run
go test ./....
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/config/config.go | Updates wording/documentation around resolving app identifiers from flags/config/default file. |
| cmd/client.go | Introduces restClient interface for dependency injection in command logic and tests. |
| cmd/mock_test.go | Adds a mock REST client and helpers for command unit tests. |
| cmd/init.go | Extracts runInit and writeRuntimeConfig for testability; updates help text to “app ID”. |
| cmd/init_test.go | Adds coverage for init error/success paths and config output behavior. |
| cmd/get.go | Extracts runGet and updates output handling and help text for “app ID”. |
| cmd/get_test.go | Adds coverage for get resolution via flags/config/default file and revision query param. |
| cmd/deploy.go | Extracts runDeploy and updates help text for “app ID”; keeps zip + upload behavior. |
| cmd/deploy_test.go | Adds coverage for deploy flag validation, URL/query construction, and error handling. |
| cmd/delete.go | Converts delete to RunE, adds runDelete, updates help text for “app ID”. |
| cmd/delete_test.go | Adds coverage for delete URL/query construction and error handling. |
| cmd/create.go | Major refactor: adds --name, --visibility, --init, response parsing, and runCreate. |
| cmd/create_test.go | Adds coverage for create inputs, body encoding, --init, --name, and visibility. |
| .github/workflows/ci.yml | Adds CI workflow to build and run Go tests on PRs/pushes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // ResolveAppName resolves the app ID using the priority order: | ||
| // 1. appFlag (--app) if provided | ||
| // 2. configPath (--config) if provided | ||
| // 3. runtime.config.json in current directory if it exists | ||
| // Returns an error if no app name can be resolved | ||
| // Returns an error if no app ID can be resolved | ||
| func ResolveAppName(appFlag, configPath string) (string, error) { |
There was a problem hiding this comment.
The docstring says this resolves an app ID, but the function is still named ResolveAppName and local variables in this function are still called appName. Either rename the function/variables to use “ID” terminology (and update call sites) or keep the “name” wording in the comments to avoid confusion.
| deployDir := filepath.Join(tmp, "dist") | ||
| os.MkdirAll(deployDir, 0755) | ||
| os.WriteFile(filepath.Join(deployDir, "index.html"), []byte("<html></html>"), 0644) | ||
|
|
There was a problem hiding this comment.
This test ignores errors from os.MkdirAll/os.WriteFile. If the filesystem operation fails, the test may fail later with a less actionable error (or behave unexpectedly). Prefer require.NoError on these calls to make failures clearer.
| configPath := filepath.Join(tmp, "my-config.json") | ||
| os.WriteFile(configPath, []byte(`{"app":"config-app"}`), 0644) | ||
|
|
There was a problem hiding this comment.
This test ignores the error return from os.WriteFile. Prefer asserting the write succeeded (e.g., with require.NoError) so failures are reported at the right place.
| err = client.Delete(deleteUrl, &response) | ||
|
|
||
| response, err := runDelete(client, deleteCmdFlags) | ||
| if err != nil { |
There was a problem hiding this comment.
On failure, runDelete returns a response string alongside the error, but RunE drops that response (return err). If the API returns a useful error body, it will no longer be surfaced to users. Consider either embedding the response body into the returned error, or simplifying runDelete to return only an error if the response is never intended to be shown.
| if err != nil { | |
| if err != nil { | |
| if response != "" { | |
| return fmt.Errorf("%w: %s", err, response) | |
| } |
Updates to improve usability of the CLI:
createwithout an ID (relies on API change shipping first)--visibilityand--nameto set those properties oncreatecommand.--initoption oncreatecommand to do both at oncecreatecommand