feat: propagate ALL_PROXY to HTTP_PROXY/HTTPS_PROXY#74
Merged
Conversation
Extend proxy support based on issue #69 feedback: - Propagate ALL_PROXY to both HTTP_PROXY and HTTPS_PROXY - Auto-configure NO_PROXY for AWS credential endpoints: - 169.254.169.254 (EC2 IMDS) - 169.254.170.2 (ECS Task Role) - 169.254.170.23 (EKS Pod Identity) - Preserve existing NO_PROXY entries, only add missing endpoints Refs #69, #67
- NO_PROXY now only includes EC2 IMDS (169.254.169.254) by default - ECS/EKS endpoints can be added via config (TODO #67) - Remove proxy URL from log output to avoid credential exposure
configureNoProxy() now runs independently when any proxy is set, not tied to ALL_PROXY propagation. Cleaner separation of concerns.
This comment was marked as resolved.
This comment was marked as resolved.
Code ReviewThank you for this PR! Overall implementation is solid with excellent test coverage. Strengths
Issues & Suggestions1. Discrepancy with Issue #69 (Medium Priority) Issue #69 states: propagate ALL_PROXY to HTTPS_PROXY only (since AWS uses HTTPS). Your implementation propagates to both HTTP_PROXY and HTTPS_PROXY. Was this intentional? Please clarify in PR description. Location: cmd/claws/main.go:191-197 2. Misleading PR Note (Low Priority) PR mentions "Propagation skipped for --help/--version" but this happens automatically since parseFlags() exits early. Consider removing this note or adding code comment for clarity. Location: cmd/claws/main.go:22-24 3. Test Documentation (Very Low Priority) The lowercase all_proxy test serves as documentation rather than regression prevention. Consider adding a comment noting this. Location: cmd/claws/proxy_test.go:58-62 Security & Performance
Best PracticesAll good: table-driven tests, proper isolation, helper functions, error handling Additional Suggestions
SummaryWell-implemented with excellent tests. Main question is HTTP_PROXY necessity. Once clarified, ready to merge! |
This was referenced Jan 2, 2026
Merged
yimsk
added a commit
that referenced
this pull request
Jan 2, 2026
* Align naming conventions with AWS CLI (#71) * Align registry names with AWS CLI conventions Rename 7 services to match AWS CLI naming: - computeoptimizer → compute-optimizer - cognito → cognito-idp - config → configservice - costexplorer → ce - eventbridge → events - macie → macie2 - sfn → stepfunctions Old names preserved as aliases for backward compatibility. Closes #65 * Fix pseudo-ARN prefix to match registry service name (ce) * Add comment explaining pseudo-ARN format for Cost Explorer * Rename directories to align with AWS CLI conventions - Service dirs: costexplorer→ce, eventbridge→events, sfn→stepfunctions, cognito→cognito-idp, config→configservice, macie→macie2, computeoptimizer→compute-optimizer, servicequotas→service-quotas, licensemanager→license-manager, networkfirewall→network-firewall - Resource dirs: hyphenate compound names (e.g., loggroups→log-groups) - Update imports and ARN mappings Closes #65 * Rename bedrock dirs to kebab-case for AWS CLI consistency * Fix DAO error strings and deduplicate client helpers * Fix error strings and deduplicate client helpers * Fix package names to match directory names (events, stepfunctions) * Add develop branch to CI triggers * feat: propagate ALL_PROXY to HTTP_PROXY/HTTPS_PROXY (#74) * feat: support ALL_PROXY environment variable (#69) * feat: propagate ALL_PROXY to HTTP_PROXY and configure NO_PROXY Extend proxy support based on issue #69 feedback: - Propagate ALL_PROXY to both HTTP_PROXY and HTTPS_PROXY - Auto-configure NO_PROXY for AWS credential endpoints: - 169.254.169.254 (EC2 IMDS) - 169.254.170.2 (ECS Task Role) - 169.254.170.23 (EKS Pod Identity) - Preserve existing NO_PROXY entries, only add missing endpoints Refs #69, #67 * fix: default NO_PROXY to IMDS only and remove proxy value from logs - NO_PROXY now only includes EC2 IMDS (169.254.169.254) by default - ECS/EKS endpoints can be added via config (TODO #67) - Remove proxy URL from log output to avoid credential exposure * refactor: simplify splitNoProxy and use tagged switch in parseFlags * refactor: separate NO_PROXY config from ALL_PROXY propagation configureNoProxy() now runs independently when any proxy is set, not tied to ALL_PROXY propagation. Cleaner separation of concerns. * refactor: remove lowercase env var support for simplicity * refactor: remove NO_PROXY auto-configuration * refactor: skip proxy propagation for --help/--version and cleanup test * test: add test case for lowercase all_proxy not supported * feat: add alias completion in command mode (#76) * feat: add alias completion in command mode (#70) Include aliases in GetSuggestions() so :cost+Tab suggests cost-explorer. Self-referential aliases (sfn→sfn) excluded from suggestions. * refactor: use prefix+fuzzy for diff completion * refactor: use CutPrefix and cache GetAliases * refactor: extract match functions to shared module - Move fuzzyMatch and matchNamesWithFallback to match.go - Make :tag/:tags clear behavior explicit - Sort suggestions alphabetically (services + aliases) * refactor: cache GetAliasesForService and sort match results * refactor: unify command parser pattern for sort and login * test: add cache concurrency tests and improve comments * refactor: use CutPrefix in ecs tasks render * refactor: use CutSuffix in parseNumericValue * fix: quote profile name in error message for clarity * fix: lowercase pattern in fuzzyMatch for case insensitivity * chore: remove .claude symlink * chore: add .claude to gitignore * fix: lowercase pattern in matchNamesWithFallback prefix check * fix: return defensive copy from alias cache methods
Merged
yimsk
added a commit
that referenced
this pull request
Jan 2, 2026
* Align naming conventions with AWS CLI (#71) * Align registry names with AWS CLI conventions Rename 7 services to match AWS CLI naming: - computeoptimizer → compute-optimizer - cognito → cognito-idp - config → configservice - costexplorer → ce - eventbridge → events - macie → macie2 - sfn → stepfunctions Old names preserved as aliases for backward compatibility. Closes #65 * Fix pseudo-ARN prefix to match registry service name (ce) * Add comment explaining pseudo-ARN format for Cost Explorer * Rename directories to align with AWS CLI conventions - Service dirs: costexplorer→ce, eventbridge→events, sfn→stepfunctions, cognito→cognito-idp, config→configservice, macie→macie2, computeoptimizer→compute-optimizer, servicequotas→service-quotas, licensemanager→license-manager, networkfirewall→network-firewall - Resource dirs: hyphenate compound names (e.g., loggroups→log-groups) - Update imports and ARN mappings Closes #65 * Rename bedrock dirs to kebab-case for AWS CLI consistency * Fix DAO error strings and deduplicate client helpers * Fix error strings and deduplicate client helpers * Fix package names to match directory names (events, stepfunctions) * Add develop branch to CI triggers * feat: propagate ALL_PROXY to HTTP_PROXY/HTTPS_PROXY (#74) * feat: support ALL_PROXY environment variable (#69) * feat: propagate ALL_PROXY to HTTP_PROXY and configure NO_PROXY Extend proxy support based on issue #69 feedback: - Propagate ALL_PROXY to both HTTP_PROXY and HTTPS_PROXY - Auto-configure NO_PROXY for AWS credential endpoints: - 169.254.169.254 (EC2 IMDS) - 169.254.170.2 (ECS Task Role) - 169.254.170.23 (EKS Pod Identity) - Preserve existing NO_PROXY entries, only add missing endpoints Refs #69, #67 * fix: default NO_PROXY to IMDS only and remove proxy value from logs - NO_PROXY now only includes EC2 IMDS (169.254.169.254) by default - ECS/EKS endpoints can be added via config (TODO #67) - Remove proxy URL from log output to avoid credential exposure * refactor: simplify splitNoProxy and use tagged switch in parseFlags * refactor: separate NO_PROXY config from ALL_PROXY propagation configureNoProxy() now runs independently when any proxy is set, not tied to ALL_PROXY propagation. Cleaner separation of concerns. * refactor: remove lowercase env var support for simplicity * refactor: remove NO_PROXY auto-configuration * refactor: skip proxy propagation for --help/--version and cleanup test * test: add test case for lowercase all_proxy not supported * feat: add alias completion in command mode (#76) * feat: add alias completion in command mode (#70) Include aliases in GetSuggestions() so :cost+Tab suggests cost-explorer. Self-referential aliases (sfn→sfn) excluded from suggestions. * refactor: use prefix+fuzzy for diff completion * refactor: use CutPrefix and cache GetAliases * refactor: extract match functions to shared module - Move fuzzyMatch and matchNamesWithFallback to match.go - Make :tag/:tags clear behavior explicit - Sort suggestions alphabetically (services + aliases) * refactor: cache GetAliasesForService and sort match results * refactor: unify command parser pattern for sort and login * test: add cache concurrency tests and improve comments * refactor: use CutPrefix in ecs tasks render * refactor: use CutSuffix in parseNumericValue * fix: quote profile name in error message for clarity * fix: lowercase pattern in fuzzyMatch for case insensitivity * chore: remove .claude symlink * chore: add .claude to gitignore * fix: lowercase pattern in matchNamesWithFallback prefix check * fix: return defensive copy from alias cache methods
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
1 participant
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.
Summary
ALL_PROXYenv var toHTTP_PROXY/HTTPS_PROXYsince Go's net/http ignores ALL_PROXYCloses #69
Changes
cmd/claws/main.go: AddpropagateAllProxy()functioncmd/claws/proxy_test.go: New test file with table-driven testsNotes
--help/--version