Conversation
* feat: support multiple -p/-r flags with comma separation * docs: update demo and docs for multi-profile/region CLI flags * refactor: extract parseFlagsFromArgs for test isolation * fix: support -p __env_only__ and __sdk_default__ special IDs
Code Review - PR #112: Release: multiple -p/-r flags supportSummaryThis PR implements support for multiple profiles and regions via comma-separated values or repeated flags. The changes are well-structured with good test coverage. Overall, this is high-quality work, but there are a few important issues to address. Critical Issues1. Validation bypassed for special profile IDs (Security/Bug)Location: cmd/claws/main.go:43-49 The validation loop rejects special profile IDs like env_only and sdk_default because they contain underscores, which pass the regex but the validation error message suggests only alphanumeric, hyphen, underscore, period are valid. However, looking at the code:
Issue: The validation will incorrectly reject these special profile IDs when passed via CLI flags. Recommendation: Update the validation to explicitly allow these special profile IDs or adjust IsValidProfileName to handle them. Code Quality Issues2. Assignment in condition (Code Quality)Location: cmd/claws/main.go:135, 144 While this works, using assignment within the condition can reduce readability. Consider separating the assignment from the condition check for better readability and easier debugging. 3. Missing edge case tests (Test Coverage)Location: cmd/claws/main_test.go The tests are comprehensive but missing a few edge cases:
Recommendation: Add tests for these scenarios to ensure robust handling. 4. Flag parsing may consume next flag as value (Bug)Location: cmd/claws/main.go:131-148 If a user provides -p or -r as the last argument or followed by another flag, the code will silently consume the next argument as a value. For example: claws -p -r us-east-1 would treat -r as the profile name. Recommendation: Add validation to check if the next arg is a flag before consuming it. Performance Considerations5. O(n²) complexity with slices.Contains (Performance)Location: cmd/claws/main.go:135, 144 Using slices.Contains in a loop creates O(n²) complexity for deduplication. For typical use cases (few profiles/regions), this is fine. However, consider using a map for better performance with many values. Priority: Low (only matters with many values) Positive Observations✅ Excellent test coverage - The new tests are well-structured and cover most scenarios Security ConsiderationsNo significant security issues identified. The validation functions IsValidProfileName and IsValidRegion provide good input sanitization, though see issue #1 about special profile IDs. Minor Suggestions
RecommendationRequest changes - Address issue #1 (special profile ID validation) and issue #4 (flag value validation) before merging. The other issues are nice-to-haves that can be addressed in follow-up work if preferred. Great work overall! This is a useful feature with solid implementation. |
Changes from develop
__env_only__and__sdk_default__special IDsMerge with merge commit (not squash)