refactor(eva): registry and expose profile selection + chore(cli): drop deprecated profile flag #122
Conversation
There was a problem hiding this comment.
Pull Request Overview
This pull request refactors the evaluation system to introduce a registry-based architecture with pluggable profiles and checks. The key changes include creating a new engine and registry system for managing security checks, adding init functions to auto-register checks, and updating the CLI to support profile selection.
- Introduces a registry-based evaluation engine with three profiles (basic, extended, additional) and check registration system
- Migrates existing evaluation functions to use auto-registration via init() functions
- Updates CLI to accept
--profileflag for selecting evaluation profiles
Reviewed Changes
Copilot reviewed 25 out of 26 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/evaluate/engine.go | New evaluation engine with Context, Check, Category, Profile, and Evaluator types |
| pkg/evaluate/registry.go | New check registration system with thread-safe registry and profile building logic |
| pkg/evaluate/categories.go | Category specifications with ordering and default profile assignments |
| pkg/evaluate/evaluate.go | Simplified wrapper functions to use new Evaluator.RunProfile API |
| pkg/evaluate/*.go (13 files) | Added init() functions to auto-register existing checks with categories |
| pkg/evaluate/evaluate_test.go | New tests for profile registration, composition, and execution |
| pkg/cli/parse.go | Updated to support --profile flag and commented out auto-escape feature |
| pkg/cli/banner.go | Updated help text with profile option and removed auto-escape documentation |
| pkg/cli/parse_test.go | Extracted timeout constant and updated logging format |
| conf/evaluate_conf.go | Added Volcano Engine cloud provider metadata API |
| README.md | Removed auto-escape documentation and added profile option |
| .gitignore | Added .cache/ directory |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // if Args["auto-escape"].(bool) { | ||
| // plugin.RunSingleTask("auto-escape") | ||
| // return true | ||
| // } | ||
|
|
There was a problem hiding this comment.
Commented-out code should be removed rather than left in the codebase. If the auto-escape feature is being deleted as the comment suggests, remove these lines entirely. If it's temporarily disabled for testing, add a TODO comment with context and a tracking issue.
| // if Args["auto-escape"].(bool) { | |
| // plugin.RunSingleTask("auto-escape") | |
| // return true | |
| // } |
| // { | ||
| // name: "./cdk eva --profile=additional", | ||
| // args: []string{"./cdk_cli_path", "eva", "--profile=additional"}, | ||
| // successStr: "randomize_va_space", | ||
| // }, |
There was a problem hiding this comment.
Commented-out test case should either be enabled or removed. If this test is being temporarily disabled, add a comment explaining why and add a TODO or tracking issue to re-enable it.
| var BannerContainerTpl = BannerHeader + ` | ||
| %s | ||
| cdk eva | ||
| cdk eva --full |
There was a problem hiding this comment.
[nitpick] The usage examples show 'cdk eva' and 'cdk eva --full' before 'cdk evaluate [--full]', but the new --profile flag is only documented in the Options section. Consider adding a usage example like 'cdk eva --profile=extended' to demonstrate the new feature.
| cdk eva --full | |
| cdk eva --full | |
| cdk eva --profile=extended |
No description provided.