feat: Add dual output mode support and E2E testing framework #461
Conversation
- Add global output format configuration via BW_OUTPUT_FORMAT env var - Add OutputFormat field to config file - Add JSON output support for show commands: - collection, database, partition, replica - alias, checkpoint, user, compaction - Framework changes: - Add FormatProvider interface for global format resolution - Add MarshalJSON helper function in resultset.go - Support global format fallback in command execution Signed-off-by: zhuwenxing <[email protected]>
- Add GetGlobalFormat() method to InstanceState to implement FormatProvider - Add GetGlobalFormat() method to ComponentShow for consistency - Fix format priority: command param > global config > default - Add FormatUnset constant to distinguish unset from default format - Update NameFormat to return FormatUnset when name is empty Signed-off-by: zhuwenxing <[email protected]>
Signed-off-by: zhuwenxing <[email protected]>
- Add GitHub Actions workflow for E2E tests - Deploys Milvus standalone with etcd and minio - Builds birdwatcher and runs all CLI commands - Collects logs on failure and uploads as artifacts - Add test data setup script (Python/pymilvus) - Creates test collections, partitions, indexes - Inserts sample data for testing - Add comprehensive CLI command tests - Show commands: database, collections, segment, session, etc. - Instance commands: healthzcheck, getconfiguration, etc. - Repair commands: dry-run mode for safety - Remove commands: dry-run mode for safety Triggered on: - Pull requests to main branch - Push to main branch - Manual workflow dispatch Signed-off-by: zhuwenxing <[email protected]>
Signed-off-by: zhuwenxing <[email protected]>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: zhuwenxing The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Summary of ChangesHello @zhuwenxing, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the usability and reliability of the CLI tool. It provides users with more flexible output options, particularly the ability to get machine-readable JSON, which is crucial for automation and integration. Concurrently, a robust E2E testing suite has been established to ensure the stability and correctness of existing and new commands, providing a safety net for future development and preventing regressions. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a significant enhancement, adding dual output mode support (including JSON) and a robust E2E testing framework. The implementation is well-structured, with a clear hierarchy for selecting the output format. The addition of E2E tests is a valuable step towards ensuring the CLI's long-term stability. My review includes suggestions to improve JSON error handling, reduce code duplication by leveraging new helper functions, and remove some redundant code for better maintainability. Overall, this is an excellent contribution.
framework/resultset.go
Outdated
| func MarshalJSON(v any) string { | ||
| bs, err := json.MarshalIndent(v, "", " ") | ||
| if err != nil { | ||
| return err.Error() |
There was a problem hiding this comment.
Returning err.Error() as a string on JSON marshaling failure will produce invalid JSON, which can break any script or tool trying to parse the output. It's better to return a valid JSON object that represents the error. This ensures consumers of the JSON output can always expect valid JSON.
errJSON := map[string]string{"error": err.Error()}
// This marshaling is unlikely to fail for a simple map.
errBs, _ := json.MarshalIndent(errJSON, "", " ")
return string(errBs)
states/etcd/show/alias.go
Outdated
| bs, err := json.MarshalIndent(output, "", " ") | ||
| if err != nil { | ||
| return err.Error() | ||
| } | ||
| return string(bs) |
There was a problem hiding this comment.
To reduce code duplication and centralize JSON marshaling, please use the new framework.MarshalJSON helper function. This also allows for consistent error handling for JSON serialization. This suggestion applies to all the other new printAsJSON methods in this pull request (checkpoint.go, collection.go, compaction.go, database.go, partition.go, replica.go, user.go).
| bs, err := json.MarshalIndent(output, "", " ") | |
| if err != nil { | |
| return err.Error() | |
| } | |
| return string(bs) | |
| return framework.MarshalJSON(output) |
| // GetGlobalFormat implements framework.FormatProvider interface. | ||
| func (c *ComponentShow) GetGlobalFormat() framework.Format { | ||
| if c.config != nil { | ||
| formatName := c.config.GetGlobalOutputFormat() | ||
| if formatName != "" { | ||
| return framework.NameFormat(formatName) | ||
| } | ||
| } | ||
| return framework.FormatDefault | ||
| } |
There was a problem hiding this comment.
This GetGlobalFormat method is redundant. ComponentShow is embedded in InstanceState, and InstanceState already provides this method by embedding framework.CmdState. The FormatProvider check is performed on the InstanceState object, so this implementation is unused (dead code) and can be removed to improve clarity.
states/etcd/show/user.go
Outdated
| if err != nil { | ||
| fmt.Println("failed to list database info", err.Error()) | ||
| return nil, errors.Wrap(err, "failed to list database info") | ||
| fmt.Println("failed to list user info", err.Error()) |
There was a problem hiding this comment.
This fmt.Println is redundant because the error is wrapped and returned on the next line. The command execution framework is responsible for printing the final error. Removing this line will prevent duplicate error messages from being displayed to the user.
| // GetGlobalFormat implements framework.FormatProvider interface. | ||
| func (s *InstanceState) GetGlobalFormat() framework.Format { | ||
| if s.config != nil { | ||
| formatName := s.config.GetGlobalOutputFormat() | ||
| if formatName != "" { | ||
| return framework.NameFormat(formatName) | ||
| } | ||
| } | ||
| return framework.FormatDefault | ||
| } |
There was a problem hiding this comment.
This GetGlobalFormat method is a duplicate of the one in framework.CmdState. Since InstanceState embeds *framework.CmdState, it already inherits this method and satisfies the FormatProvider interface. You can remove this redundant implementation to avoid code duplication.
Signed-off-by: zhuwenxing <[email protected]>
Signed-off-by: zhuwenxing <[email protected]>
- Return valid JSON object on marshaling error - Use framework.MarshalJSON helper in show commands - Remove unused json import from show files - Remove redundant fmt.Println in user.go - Fix bash arithmetic increment with set -e (add || true) Signed-off-by: zhuwenxing <[email protected]>
Signed-off-by: zhuwenxing <[email protected]>
Signed-off-by: zhuwenxing <[email protected]>
Signed-off-by: zhuwenxing <[email protected]>
Signed-off-by: zhuwenxing <[email protected]>
Signed-off-by: zhuwenxing <[email protected]>
Signed-off-by: zhuwenxing <[email protected]>
- Add detailed command output logging to e2e_logs/ directory - Generate JSON test report with structured results - Record execution time and output line count for each test - Enhance test data setup with: - Multiple collections (test, simple, binary, sparse) - Multiple partitions and segments via batch inserts - Compaction trigger for compaction task data - Additional database creation - Add more JSON format tests for show commands - Update workflow to upload detailed logs as artifacts Signed-off-by: zhuwenxing <[email protected]>
- Add user.yaml config to enable authorizationEnabled - Mount user.yaml in docker-compose for Milvus - Connect with root credentials in setup_test_data.py - Create multiple test users and roles - Add role grant and list operations Signed-off-by: zhuwenxing <[email protected]>
…compaction commands Signed-off-by: zhuwenxing <[email protected]>
Signed-off-by: zhuwenxing <[email protected]>
Signed-off-by: zhuwenxing <[email protected]>
…source-group, stats-task, bulkinsert commands Signed-off-by: zhuwenxing <[email protected]>
…lection ID Signed-off-by: zhuwenxing <[email protected]>
2 participants
Summary
This PR introduces two major features:
1. Dual Output Mode Support
BW_OUTPUT_FORMATenvironment variable--formatflag overrideshow databaseshow collectionsshow sessionshow partitionshow checkpointshow aliasshow usershow replicashow compaction2. E2E Testing Framework
Changes
Test Plan
BW_OUTPUT_FORMAT=json