Skip to content

Add --project-id and --organization-id flags to target command#44

Merged
austin-denoble merged 6 commits intomainfrom
adenoble/update-target-cmd-options
Oct 9, 2025
Merged

Add --project-id and --organization-id flags to target command#44
austin-denoble merged 6 commits intomainfrom
adenoble/update-target-cmd-options

Conversation

@austin-denoble
Copy link
Collaborator

@austin-denoble austin-denoble commented Oct 8, 2025 •
edited
Loading

Problem

For projects and organizations we use ID values in all commands in the CLI except for target. This command was originally written before finalizing the public API definitions, and went with an ease-of-use UX of using either --proj (for project name), or --org (for organization name) to target. This is convenient, but could be confusing given the usage of project and organization IDs elsewhere across the admin API.

Solution

Align the pc target command around resource IDs rather than names. I've kept the ability to use names for now. You'll need to pass one or the other for organization or project.

I also did a bit of cleanup of package level variables in this PR. We don't need to be exporting the struct defining command options from each package, so standardize that.

This PR also resolves bugs related to clearing default API key values. We were storing the key in two different places (state, and secrets) and using one or the other for the presentational layer. I've also made sure when you're clearing service account or default API key credentials, the auth context will resolve properly.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update
  • Infrastructure change (CI configs, etc)
  • Non-code change (docs, etc)
  • None of the above: (explain here)

Test Plan

Test that the target command still works as expected:

$ pc target --proj "my-project"
$ pc target --project-id "id-value"
$ pc target --org "my-org"
$ pc target --organization-id "id-value"

$ pc target --project-id "id-value" --organization-id "id-value"

# mixing and matching should work
$ pc target --proj "proj-name" --organization-id "id-value"

# errors - no flags, both name and ID, etc

@claude
Copy link

claude bot commented Oct 8, 2025 •
edited
Loading

Claude finished @austin-denoble's task —— View job

Code Review Complete ✅

Summary

This PR successfully adds --project-id and --organization-id flags to the target command while maintaining backward compatibility with existing --org and --project name-based flags. The implementation is well-structured and includes proper validation and error handling.

✅ Strengths

1. Clean API Design

  • Maintains backward compatibility with existing name-based flags (--org, --project)
  • Adds consistent ID-based flags (--organization-id, --project-id) aligned with other CLI commands
  • Proper mutual exclusion validation prevents conflicting flag combinations

2. Good Error Handling & User Experience

  • Clear validation in validateTargetOptions function at target.go:278-290
  • Informative error messages with available options when resources aren't found
  • Helper functions getOrgForTarget and getProjectForTarget provide consistent lookup logic

3. Consistent Code Cleanup

  • Successfully makes command option structs private across all packages (good encapsulation)
  • Maintains consistent naming patterns (createProjectCmdOptions, createAPIKeyOptions, etc.)

4. Robust Implementation

  • Handles both interactive and programmatic usage patterns
  • Proper state management with organization/project context
  • Maintains authentication flow when switching organizations

⚠️ Areas for Improvement

1. Missing Test Coverage ⚠️

  • No unit tests for the new validation logic (validateTargetOptions)
  • No tests for the new lookup functions (getOrgForTarget, getProjectForTarget)
  • These functions contain important business logic that should be tested
  • Existing test pattern in internal/pkg/cli/command/index/index_test.go could be used as a template

2. Minor Issues

Example Inconsistency (target.go:57)

# Example shows old format:
pc target --org "organization-name" -project "project-name"
# Should be:
pc target --org "organization-name" --project "project-name"

Potential Edge Cases

  • Empty string validation: Functions should handle empty string inputs (though current validation prevents this)
  • Case sensitivity: Consider whether name matching should be case-insensitive for better UX

🔒 Security Assessment

No security concerns identified:

  • No credential exposure in error messages
  • Proper token handling and re-authentication flow
  • Input validation prevents injection-style attacks

🚀 Performance Considerations

Good performance characteristics:

  • Efficient linear search through organizations/projects (acceptable for typical CLI usage)
  • Minimal API calls - fetches lists once and reuses them
  • Proper client re-instantiation when auth context changes

📋 Recommendations

  1. Add Unit Tests (High Priority)

    func TestValidateTargetOptions(t *testing.T) {
    // Test cases for mutual exclusion validation
}

func TestGetOrgForTarget(t *testing.T) {
    // Test cases for name/ID lookup logic
}

  2. Fix Example Format (Low Priority)

    • Update the command example to use --project instead of -project

  3. Consider Enhancements (Optional)

    • Case-insensitive name matching for better UX
    • Fuzzy matching with suggestions for typos

🎯 Overall Assessment

Excellent work! This PR successfully achieves its goals with clean, maintainable code. The implementation is robust and user-friendly. The main gap is test coverage for the new validation and lookup logic, which is important given these functions handle core business logic.

Approval recommendation: ✅ Approve with suggestion to add tests in a follow-up PR.

@claude
Copy link

claude bot commented Oct 8, 2025 •
edited
Loading

Claude finished @austin-denoble's task —— View job

Code Review Complete ✅

Summary

This PR successfully adds --project-id and --organization-id flags to the target command while maintaining backward compatibility with existing --org and --project name-based flags. The implementation is well-structured and includes proper validation and error handling.

✅ Strengths

1. Clean API Design

  • Maintains backward compatibility with existing name-based flags (--org, --project)
  • Adds consistent ID-based flags (--organization-id, --project-id) aligned with other CLI commands
  • Proper mutual exclusion validation prevents conflicting flag combinations

2. Good Error Handling & User Experience

  • Clear validation in validateTargetOptions function at target.go:285-296
  • Informative error messages with available options when resources aren't found
  • Helper functions getOrgForTarget and getProjectForTarget provide consistent lookup logic

3. Consistent Code Cleanup

  • Successfully makes command option structs private across all packages (good encapsulation)
  • Maintains consistent naming patterns (createProjectCmdOptions, createAPIKeyOptions, etc.)

4. Robust Implementation

  • Handles both interactive and programmatic usage patterns
  • Proper state management with organization/project context
  • Maintains authentication flow when switching organizations

⚠️ Areas for Improvement

1. Missing Test Coverage ⚠️

  • No unit tests for the new validation logic (validateTargetOptions)
  • No tests for the new lookup functions (getOrgForTarget, getProjectForTarget)
  • These functions contain important business logic that should be tested
  • Existing test pattern in internal/pkg/cli/command/index/index_test.go could be used as a template

2. Minor Issues

Example Inconsistency (target.go:54)

# Example shows old format:
pc target --org "organization-name" -project "project-name"
# Should be:
pc target --org "organization-name" --project "project-name"

Potential Edge Cases

  • Empty string validation: Functions should handle empty string inputs (though current validation prevents this)
  • Case sensitivity: Consider whether name matching should be case-insensitive for better UX

🔒 Security Assessment

No security concerns identified:

  • No credential exposure in error messages
  • Proper token handling and re-authentication flow
  • Input validation prevents injection-style attacks

🚀 Performance Considerations

Good performance characteristics:

  • Efficient linear search through organizations/projects (acceptable for typical CLI usage)
  • Minimal API calls - fetches lists once and reuses them
  • Proper client re-instantiation when auth context changes

📋 Recommendations

  1. Add Unit Tests (High Priority)

    func TestValidateTargetOptions(t *testing.T) {
    // Test cases for mutual exclusion validation
}

func TestGetOrgForTarget(t *testing.T) {
    // Test cases for name/ID lookup logic
}

  2. Fix Example Format (Low Priority)

    • Update the command example to use --project instead of -project

  3. Consider Enhancements (Optional)

    • Case-insensitive name matching for better UX
    • Fuzzy matching with suggestions for typos

🎯 Overall Assessment

Excellent work! This PR successfully achieves its goals with clean, maintainable code. The implementation is robust and user-friendly. The main gap is test coverage for the new validation and lookup logic, which is important given these functions handle core business logic.

Approval recommendation: ✅ Approve with suggestion to add tests in a follow-up PR.

@claude
Copy link

claude bot commented Oct 9, 2025 •
edited
Loading

Claude finished @austin-denoble's task —— View job

Code Review Complete ✅

Summary

This PR successfully adds --project-id and --organization-id flags to the target command while maintaining backward compatibility with existing --org and --project name-based flags. The implementation is well-structured and includes proper validation and error handling.

✅ Strengths

1. Clean API Design

  • Maintains backward compatibility with existing name-based flags (--org, --project)
  • Adds consistent ID-based flags (--organization-id, --project-id) aligned with other CLI commands
  • Proper mutual exclusion validation prevents conflicting flag combinations

2. Good Error Handling & User Experience

  • Clear validation in validateTargetOptions function at target.go:292-303
  • Informative error messages with available options when resources aren't found
  • Helper functions getOrgForTarget and getProjectForTarget provide consistent lookup logic at lines 306-390

3. Consistent Code Cleanup

  • Successfully makes command option structs private across all packages (good encapsulation)
  • Maintains consistent naming patterns (createAPIKeyOptions, targetCmdOptions, etc.)

4. Robust Implementation

  • Handles both interactive and programmatic usage patterns
  • Proper state management with organization/project context
  • Maintains authentication flow when switching organizations (lines 161-169, 210-218)

⚠️ Areas for Improvement

1. Missing Test Coverage ⚠️

  • No unit tests for the new validation logic (validateTargetOptions)
  • No tests for the new lookup functions (getOrgForTarget, getProjectForTarget)
  • These functions contain important business logic that should be tested
  • Existing test pattern in internal/pkg/cli/command/index/index_test.go could be used as a template

2. Minor Issues

Example Inconsistency (target.go:54)

# Example shows old format:
pc target --org "organization-name" -project "project-name"
# Should be:
pc target --org "organization-name" --project "project-name"

Potential Edge Cases

  • Empty string validation: Functions should handle empty string inputs (though current validation prevents this)
  • Case sensitivity: Consider whether name matching should be case-insensitive for better UX

🔒 Security Assessment

No security concerns identified:

  • No credential exposure in error messages
  • Proper token handling and re-authentication flow
  • Input validation prevents injection-style attacks

🚀 Performance Considerations

Good performance characteristics:

  • Efficient linear search through organizations/projects (acceptable for typical CLI usage)
  • Minimal API calls - fetches lists once and reuses them
  • Proper client re-instantiation when auth context changes

📋 Recommendations

  1. Add Unit Tests (High Priority)

    func TestValidateTargetOptions(t *testing.T) {
    // Test cases for mutual exclusion validation
}

func TestGetOrgForTarget(t *testing.T) {
    // Test cases for name/ID lookup logic
}

  2. Fix Example Format (Low Priority)

    • Update the command example to use --project instead of -project

  3. Consider Enhancements (Optional)

    • Case-insensitive name matching for better UX
    • Fuzzy matching with suggestions for typos

🎯 Overall Assessment

Excellent work! This PR successfully achieves its goals with clean, maintainable code. The implementation is robust and user-friendly. The main gap is test coverage for the new validation and lookup logic, which is important given these functions handle core business logic.

Approval recommendation: ✅ Approve with suggestion to add tests in a follow-up PR.

@claude
Copy link

claude bot commented Oct 9, 2025 •
edited
Loading

Claude finished @austin-denoble's task —— View job

Code Review Complete ✅

Summary

This PR successfully adds --project-id and --organization-id flags to the target command while maintaining backward compatibility with existing --org and --project name-based flags. The implementation is well-structured and includes proper validation and error handling.

✅ Strengths

1. Clean API Design

  • Maintains backward compatibility with existing name-based flags (--org, --project)
  • Adds consistent ID-based flags (--organization-id, --project-id) aligned with other CLI commands
  • Proper mutual exclusion validation prevents conflicting flag combinations

2. Good Error Handling & User Experience

  • Clear validation in validateTargetOptions function at target.go:292-303
  • Informative error messages with available options when resources aren't found
  • Helper functions getOrgForTarget and getProjectForTarget provide consistent lookup logic at lines 306-390

3. Consistent Code Cleanup

  • Successfully makes command option structs private across all packages (good encapsulation)
  • Maintains consistent naming patterns (createAPIKeyOptions, targetCmdOptions, etc.)

4. Robust Implementation

  • Handles both interactive and programmatic usage patterns
  • Proper state management with organization/project context
  • Maintains authentication flow when switching organizations (lines 161-169, 210-218)

⚠️ Areas for Improvement

1. Missing Test Coverage ⚠️

  • No unit tests for the new validation logic (validateTargetOptions)
  • No tests for the new lookup functions (getOrgForTarget, getProjectForTarget)
  • These functions contain important business logic that should be tested
  • Existing test pattern in internal/pkg/cli/command/index/index_test.go could be used as a template

2. Minor Issues

Example Inconsistency (target.go:54)

# Example shows old format:
pc target --org "organization-name" -project "project-name"
# Should be:
pc target --org "organization-name" --project "project-name"

Potential Edge Cases

  • Empty string validation: Functions should handle empty string inputs (though current validation prevents this)
  • Case sensitivity: Consider whether name matching should be case-insensitive for better UX

🔒 Security Assessment

No security concerns identified:

  • No credential exposure in error messages
  • Proper token handling and re-authentication flow
  • Input validation prevents injection-style attacks

🚀 Performance Considerations

Good performance characteristics:

  • Efficient linear search through organizations/projects (acceptable for typical CLI usage)
  • Minimal API calls - fetches lists once and reuses them
  • Proper client re-instantiation when auth context changes

📋 Recommendations

  1. Add Unit Tests (High Priority)

    func TestValidateTargetOptions(t *testing.T) {
    // Test cases for mutual exclusion validation
}

func TestGetOrgForTarget(t *testing.T) {
    // Test cases for name/ID lookup logic
}

  2. Fix Example Format (Low Priority)

    • Update the command example to use --project instead of -project

  3. Consider Enhancements (Optional)

    • Case-insensitive name matching for better UX
    • Fuzzy matching with suggestions for typos

🎯 Overall Assessment

Excellent work! This PR successfully achieves its goals with clean, maintainable code. The implementation is robust and user-friendly. The main gap is test coverage for the new validation and lookup logic, which is important given these functions handle core business logic.

Approval recommendation: ✅ Approve with suggestion to add tests in a follow-up PR.

@claude
Copy link

claude bot commented Oct 9, 2025 •
edited
Loading

Claude finished @austin-denoble's task —— View job

Code Review Complete ✅

Summary

This PR successfully adds --project-id and --organization-id flags to the target command while maintaining backward compatibility with existing --org and --project name-based flags. The implementation is well-structured and includes proper validation and error handling.

✅ Strengths

1. Clean API Design

  • Maintains backward compatibility with existing name-based flags (--org, --project)
  • Adds consistent ID-based flags (--organization-id, --project-id) aligned with other CLI commands
  • Proper mutual exclusion validation prevents conflicting flag combinations

2. Good Error Handling & User Experience

  • Clear validation in validateTargetOptions function at target.go:295-307
  • Informative error messages with available options when resources aren't found
  • Helper functions getOrgForTarget and getProjectForTarget provide consistent lookup logic at lines 309-393

3. Consistent Code Cleanup

  • Successfully makes command option structs private across all packages (good encapsulation)
  • Maintains consistent naming patterns (createAPIKeyOptions, targetCmdOptions, etc.)

4. Robust Implementation

  • Handles both interactive and programmatic usage patterns
  • Proper state management with organization/project context
  • Maintains authentication flow when switching organizations (lines 163-169, 213-218)

⚠️ Areas for Improvement

1. Missing Test Coverage ⚠️

  • No unit tests for the new validation logic (validateTargetOptions)
  • No tests for the new lookup functions (getOrgForTarget, getProjectForTarget)
  • These functions contain important business logic that should be tested
  • Existing test pattern in internal/pkg/cli/command/index/index_test.go could be used as a template

2. Minor Issues

Example Inconsistency (target.go:54)

# Example shows old format:
pc target --org "organization-name" -project "project-name"
# Should be:
pc target --org "organization-name" --project "project-name"

Potential Edge Cases

  • Empty string validation: Functions should handle empty string inputs (though current validation prevents this)
  • Case sensitivity: Consider whether name matching should be case-insensitive for better UX

🔒 Security Assessment

No security concerns identified:

  • No credential exposure in error messages
  • Proper token handling and re-authentication flow
  • Input validation prevents injection-style attacks

🚀 Performance Considerations

Good performance characteristics:

  • Efficient linear search through organizations/projects (acceptable for typical CLI usage)
  • Minimal API calls - fetches lists once and reuses them
  • Proper client re-instantiation when auth context changes

📋 Recommendations

  1. Add Unit Tests (High Priority)

    func TestValidateTargetOptions(t *testing.T) {
    // Test cases for mutual exclusion validation
}

func TestGetOrgForTarget(t *testing.T) {
    // Test cases for name/ID lookup logic
}

  2. Fix Example Format (Low Priority)

    • Update the command example to use --project instead of -project

  3. Consider Enhancements (Optional)

    • Case-insensitive name matching for better UX
    • Fuzzy matching with suggestions for typos

🎯 Overall Assessment

Excellent work! This PR successfully achieves its goals with clean, maintainable code. The implementation is robust and user-friendly. The main gap is test coverage for the new validation and lookup logic, which is important given these functions handle core business logic.

Approval recommendation: ✅ Approve with suggestion to add tests in a follow-up PR.

@austin-denoble austin-denoble merged commit d191dea into main Oct 9, 2025
5 checks passed
@austin-denoble austin-denoble deleted the adenoble/update-target-cmd-options branch October 9, 2025 04:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bradumbaugh bradumbaugh Awaiting requested review from bradumbaugh

@jhamon jhamon Awaiting requested review from jhamon

@rohanshah18 rohanshah18 Awaiting requested review from rohanshah18

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@austin-denoble