DEVOPS-688 feat: add support for clariti org pooling#5
Conversation
…force CLI plugin
… Clariti with optional pool ID
WalkthroughAdds Clariti support: a new clariti utility module, extends the Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as "cci org import"
participant Clariti as "cumulusci.utils.clariti"
participant SF as "sf CLI"
participant Keychain as "CCI Keychain"
rect rgba(220,235,255,0.45)
note over CLI,Clariti: Clariti-based checkout path (new)
User->>CLI: cci org import [USERNAME?] [--org <name>] [--pool-id <id>]
alt pool-id provided OR username omitted
CLI->>Clariti: resolve_pool_id(pool_id, project_root)
Clariti-->>CLI: pool_id or error
CLI->>Clariti: checkout_org_from_pool(pool_id, alias?)
Clariti->>SF: sf clariti org checkout --json
SF-->>Clariti: stdout (JSON) / exit status
Clariti-->>CLI: ClaritiCheckoutResult(username, alias, org_id, pool_id)
opt alias sync
CLI->>Clariti: set_sf_alias(alias, username)
Clariti->>SF: sf alias set ...
SF-->>Clariti: status
Clariti-->>CLI: success/failure
end
CLI->>Keychain: import org using username (+derived org_name)
Keychain-->>CLI: persisted
CLI-->>User: Imported org (Clariti checkout)
else
note over CLI,Keychain: Legacy keychain import (unchanged)
User->>CLI: cci org import <username_or_alias> --org <name>
CLI->>Keychain: import from SFDX keychain
Keychain-->>CLI: persisted
CLI-->>User: Imported org (legacy)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
cumulusci/utils/clariti.py (4)
107-116: Add a timeout and handle CLI hangs in Clariti checkout.External CLI calls can hang indefinitely. Add a reasonable timeout and surface a clear error on TimeoutExpired.
-def checkout_org_from_pool( - pool_id: str, - *, - alias: Optional[str] = None, - env: Optional[Dict[str, str]] = None, -) -> ClaritiCheckoutResult: +def checkout_org_from_pool( + pool_id: str, + *, + alias: Optional[str] = None, + env: Optional[Dict[str, str]] = None, + timeout: float = 300.0, +) -> ClaritiCheckoutResult: @@ - try: - proc = subprocess.run( + try: + proc = subprocess.run( command, check=False, text=True, capture_output=True, - env=env, + env=env, + timeout=timeout, ) - except FileNotFoundError as err: + except subprocess.TimeoutExpired as err: + raise ClaritiError( + f"Timed out waiting for Clariti checkout after {timeout:.0f}s." + ) from err + except FileNotFoundError as err: raise ClaritiError("Salesforce CLI 'sf' was not found on PATH.") from err @@ - try: - payload = json.loads(stdout) + try: + payload = json.loads(stdout) except json.JSONDecodeError as err: - raise ClaritiError( - "Failed to parse JSON from Clariti CLI response." - f" Raw output: {stdout}" - ) from err + # Avoid dumping unbounded output; include a short snippet for diagnostics. + snippet = stdout if len(stdout) <= 2000 else stdout[:2000] + " ... [truncated]" + raise ClaritiError( + "Failed to parse JSON from Clariti CLI response. " + f"Raw output (snippet): {snippet}" + ) from errAlso applies to: 118-136
139-151: Broaden JSON extraction to handle nested result shapes.Clariti/SFCLIs often nest data under result. Add fallbacks similar to username/alias.
- org_id_value = _extract_string( - payload, (("orgId",),), allow_missing=True - ) - instance_url_value = _extract_string( - payload, (("instanceUrl",),), allow_missing=True - ) - org_type_value = _extract_string( - payload, (("orgType",),), allow_missing=True - ) - pool_id_value = _extract_string( - payload, (("poolId",),), allow_missing=True - ) + org_id_value = _extract_string( + payload, (("orgId",), ("result", "orgId")), allow_missing=True + ) + instance_url_value = _extract_string( + payload, (("instanceUrl",), ("result", "instanceUrl")), allow_missing=True + ) + org_type_value = _extract_string( + payload, (("orgType",), ("result", "orgType")), allow_missing=True + ) + pool_id_value = _extract_string( + payload, (("poolId",), ("result", "poolId")), allow_missing=True + )
213-217: Improve error context from _extract_string.When a required field is missing, include searched paths to aid debugging.
- raise ClaritiError("Unable to determine required field from Clariti response.") + searched = [".".join(p) for p in paths] + raise ClaritiError( + "Unable to determine required field from Clariti response. " + f"Searched paths: {', '.join(searched)}" + )
171-181: Also add a timeout to alias setting.Same external-call risk applies to
sf alias set. Add a timeout and handle it.-def set_sf_alias( - alias: str, username: str, *, env: Optional[Dict[str, str]] = None -) -> Tuple[bool, Optional[str]]: +def set_sf_alias( + alias: str, + username: str, + *, + env: Optional[Dict[str, str]] = None, + timeout: float = 30.0, +) -> Tuple[bool, Optional[str]]: @@ - try: - proc = subprocess.run( + try: + proc = subprocess.run( command, check=False, text=True, capture_output=True, - env=env, + env=env, + timeout=timeout, ) + except subprocess.TimeoutExpired: + return False, f"Timed out setting alias after {timeout:.0f}s." except FileNotFoundError: return False, "Salesforce CLI 'sf' was not found on PATH."Also applies to: 183-191
cumulusci/cli/org.py (2)
312-321: Guard against accidental overwrite of an existing org name.
SfdxOrgConfig(...).save()will overwrite an existing entry. Align with other commands by invokingruntime.check_org_overwrite(org_name)first.if not username_or_alias: raise click.UsageError( "Please provide a username or alias, or specify a Clariti pool id." ) if not org_name: raise click.UsageError("Please specify ORGNAME or --org ORGNAME.") + # Avoid unintentionally overwriting an existing org entry + runtime.check_org_overwrite(org_name)
291-311: Alias alignment UX looks good; consider surfacing a hint when Clariti already set a different alias.Non-blocking: You print the Clariti-assigned alias and then set the CCI/
sfalias to org_name. That creates two aliases for the same username, which is fine. Optionally, suggest how to remove the old alias (sf alias unset <name>) if users want a single alias.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
AGENTS.md(1 hunks)cumulusci/cli/org.py(2 hunks)cumulusci/utils/clariti.py(1 hunks)
…ool_id functions
…h Clariti Org Pooling System
…nd --pool-id conflict; enhance error handling in checkout_org_from_pool and set_sf_alias
…ptional pool ID; enhance docstrings for clarity
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cumulusci/cli/org.py (1)
258-263: Reflect Click’s optional arguments in type hints.
username_or_aliasandorg_namecan beNonewhen omitted on the CLI, so the signature should useOptional[str]to match actual values and keep type checkers honest.Apply this diff:
def org_import( runtime: CliRuntime, - username_or_alias: str, - org_name: str, + username_or_alias: Optional[str], + org_name: Optional[str], pool_id: Optional[str] = None, ):
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
cumulusci/cli/org.py(2 hunks)cumulusci/utils/clariti.py(1 hunks)
…Org Pooling System support and usage instructions
…s and cci_alias options
…SERNAME_OR_ALIAS and --pool-id
…ol ID resolution
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cumulusci/tests/test_cli_org_import.py (1)
9-16: Clarify the purpose of_load_keychainor remove it.The
_load_keychainmethod is defined but never called in the test. If it's intended to override a method from a parent class that would be called during test execution, consider adding a docstring explaining this. Otherwise, remove it to reduce confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
cumulusci/tests/test_cli_org_import.py(1 hunks)cumulusci/tests/test_utils_clariti.py(1 hunks)docs/headless.md(1 hunks)docs/scratch-orgs.md(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- docs/scratch-orgs.md
🔇 Additional comments (15)
docs/headless.md (1)
125-126: LGTM! Documentation reflects the updated CLI syntax.The updated command syntax clearly specifies the alias-based import approach and provides helpful clarification about the second positional argument.
cumulusci/tests/test_cli_org_import.py (1)
18-32: LGTM! Test correctly validates mutual exclusivity.The test properly validates that the CLI rejects conflicting inputs (both username/alias and pool_id) and checks for appropriate error messaging.
cumulusci/tests/test_utils_clariti.py (13)
16-17: LGTM! Test validates explicit pool_id precedence.The test correctly verifies that an explicitly provided pool_id takes precedence over config-based resolution.
20-26: LGTM! Test validates behavior with missing config key.The test properly validates that when the config file exists but doesn't contain the required pool_id key,
Noneis returned.
29-35: LGTM! Test validates missing config file error.The test correctly verifies that a ClaritiError is raised when the config file doesn't exist and checks for appropriate error messaging.
38-60: LGTM! Test validates successful org checkout with username parsing.The test properly validates the parsing of the checkout response payload and ensures all fields are correctly extracted into the ClaritiCheckoutResult.
63-81: LGTM! Test validates nested username extraction.The test correctly verifies that usernames can be extracted from nested result structures, which handles different Clariti CLI response formats.
83-98: LGTM! Test validates pool_id omission behavior.The test properly verifies that when
pool_idisNone, the CLI command is invoked without the--pool-idflag, allowing the Clariti CLI to use its default pool configuration.
100-112: LGTM! Test validates plain text error handling.The test correctly verifies that ClaritiError is raised with appropriate error messaging when the Clariti CLI returns a non-zero exit code with plain text stderr.
114-134: LGTM! Test validates JSON-formatted error handling.The test properly verifies that structured JSON errors from the Clariti CLI are parsed and formatted into user-friendly error messages.
136-159: LGTM! Test validates debug mode error formatting.The test correctly verifies that in debug mode, the full raw JSON response is included in the error message for troubleshooting purposes.
161-172: LGTM! Test validates successful alias setting.The test properly verifies that the
sf alias setcommand is correctly constructed and executed, and that success is properly returned.
174-184: LGTM! Test validates alias setting failure.The test correctly verifies that alias setting failures are properly detected and error messages are returned.
187-188: LGTM! Test validates alias preference in default org name.The test properly verifies that when an alias is provided, it's used as the default org name rather than the username.
191-195: LGTM! Test validates username sanitization.The test correctly verifies that special characters in usernames are sanitized when building the default org name, ensuring compatibility with CumulusCI's org naming requirements.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Dipak Parmar <hi@dipak.tech>
…") decorator. Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Dipak Parmar <hi@dipak.tech>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cumulusci/utils/clariti.py (1)
179-214: Consider clarifying the error message prefix.The function correctly sets the SF CLI alias with proper error handling. However, the error message "Clariti alias failed" (line 209) might be slightly misleading since
sf alias setis a standard Salesforce CLI command, not Clariti-specific.Consider this change for clarity:
- summary = f"Clariti alias failed: {summary}" + summary = f"Failed to set SF alias: {summary}"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
cumulusci/utils/clariti.py(1 hunks)
🔇 Additional comments (8)
cumulusci/utils/clariti.py (8)
1-13: LGTM! Well-structured module with appropriate imports.The module docstring clearly describes the purpose, and all necessary imports are present. The type hints and error handling dependencies are properly imported.
15-38: LGTM! Clean exception and dataclass definitions.The
ClaritiErrorexception provides a clear, specific type for Clariti-related failures. TheClaritiCheckoutResultdataclass is well-documented with appropriate field types, clearly distinguishing required (username) from optional metadata fields.
41-70: LGTM! Solid pool ID resolution logic.The function correctly handles both explicit pool IDs and implicit configuration via
.clariti.json. The error messages are clear and actionable, guiding users to provide--pool-idor ensure the config file exists. ReturningNonewhen the config file is present correctly delegates to the Clariti CLI.
73-88: LGTM! Robust multi-path extraction strategy.Defining multiple search paths for username and alias fields provides excellent resilience against varying Clariti CLI response structures. This approach ensures the utility can adapt to different response formats without breaking.
91-176: LGTM! Comprehensive checkout implementation with excellent error handling.The function demonstrates strong practices:
- Secure subprocess execution (list arguments, no shell=True)
- Comprehensive error handling for missing CLI, subprocess failures, and JSON parsing
- Debug mode integration for detailed diagnostics
- Required vs. optional field extraction properly differentiated
- Clear error messages with context
217-246: LGTM! Robust field extraction with clear semantics.The helper correctly traverses nested dictionaries, validates types, and handles missing values appropriately. The
allow_missingparameter provides clear control over required vs. optional fields.
249-265: LGTM! Alias sanitization concern from previous review has been addressed.The function now correctly sanitizes the Clariti-provided alias (lines 258-261) using the same pattern as the username fallback, preventing issues with special characters in
sf alias setcommands and keychain filenames. The implementation matches the previous review's suggested fix exactly.Based on learnings.
268-301: LGTM! Comprehensive error message extraction.The helper effectively handles multiple error response formats by attempting JSON parsing and extracting common error fields (message, error, msg, code, name, status). The fallback behavior ensures users receive meaningful feedback regardless of response structure. Lines 299-301 correctly return the full JSON as both summary and raw output when no more concise message can be extracted.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
cumulusci/utils/clariti.py(1 hunks)
dipakparmar
force-pushed
the
feature/DEVOPS-688
branch
from
26a1f81 to
c588e7e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cumulusci/utils/clariti.py (1)
153-166: Enhance error specificity for missing required fields.When
_extract_stringfails to locate a required field, it raises a generic"Unable to determine required field from Clariti response."Without knowing which field (username, org_id, etc.) is missing, troubleshooting becomes harder. Consider passing a field name parameter to_extract_stringso the error can report the specific missing field.Apply this diff to enhance error messages:
def _extract_string( payload: Dict[str, Any], paths: Sequence[Sequence[str]], *, allow_missing: bool = False, + field_name: Optional[str] = None, ) -> Optional[str]: """Extract the first non-empty string value from the payload. :param payload: Data structure returned from Clariti. :param paths: Candidate key paths to inspect. :param allow_missing: Whether to return ``None`` when no value is found. + :param field_name: Name of the field for error reporting. :returns: The first matching string or ``None``. :raises ClaritiError: if no value is found and ``allow_missing`` is false. """ for path in paths: value: Any = payload for key in path: if isinstance(value, dict) and key in value: value = value[key] else: value = None break if isinstance(value, str) and value.strip(): return value.strip() if allow_missing: return None - raise ClaritiError("Unable to determine required field from Clariti response.") + error_msg = "Unable to determine required field from Clariti response." + if field_name: + error_msg = f"Unable to determine '{field_name}' from Clariti response." + raise ClaritiError(error_msg)Then update the call sites:
- username = cast(str, _extract_string(payload, _USERNAME_PATHS)) + username = cast(str, _extract_string(payload, _USERNAME_PATHS, field_name="username"))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
cumulusci/utils/clariti.py(1 hunks)
2 participants
Summary by CodeRabbit
New Features
Documentation
Tests