DEVOPS-735 fix: sf cli not accessible in windows for clariti commands#9
DEVOPS-735 fix: sf cli not accessible in windows for clariti commands#9dipakparmar merged 4 commits intomainfrom
Conversation
…org checkout and alias setting
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Clariti CLI integration cumulusci/utils/clariti.py |
Replace subprocess.run usage with sfdx(..., args=command_args, capture_output=True, check_return=False); add _read_process_stream and _coerce_stream_value to normalize stdout/stderr; handle OSError explicitly for missing/unexecutable CLI; refine non-zero return-code handling to raise ClaritiError with summarized or raw output depending on debug mode; inline JSON field extraction and preserve public return types. |
Tests — sfdx wrapper mocking cumulusci/tests/test_utils_clariti.py |
Replace subprocess-based test stubs with a test sfdx wrapper; add _make_proc helper to simulate returncode/stdout_text/stderr_text; update tests to assert wrapper call signature (args list) and to return mocked proc results; adjust expected error messages/branches to match new wrapper behavior. |
Sequence Diagram(s)
sequenceDiagram
participant Caller as CCI Caller
participant Clariti as clariti.py
participant SfdxWrapper as sfdx Wrapper
participant CLI as Salesforce CLI
rect rgb(245,250,255)
Note over Clariti,SfdxWrapper: New flow — args list + wrapper + normalized streams
Caller->>Clariti: checkout_org_from_pool(...)
Clariti->>SfdxWrapper: sfdx(command, args=command_args, capture_output=True, check_return=False)
alt CLI executes successfully
SfdxWrapper->>CLI: Run command
CLI-->>SfdxWrapper: proc (stdout_text/stderr_text, returncode=0)
SfdxWrapper-->>Clariti: proc
Clariti->>Clariti: parse stdout_text → JSON → fields
Clariti-->>Caller: ClaritiCheckoutResult
else CLI missing / exec error
SfdxWrapper-->>Clariti: raises OSError
Clariti-->>Caller: ClaritiError (CLI not found / cannot execute)
else non-zero return
SfdxWrapper-->>Clariti: proc with returncode != 0
Clariti->>Clariti: normalize streams, summarize or include raw output
Clariti-->>Caller: ClaritiError
end
end
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~35 minutes
- Pay extra attention to:
cumulusci/utils/clariti.py— OSError handling, debug vs non-debug error content, and correct JSON field extraction.set_sf_aliasbehavior and its(bool, Optional[str])return semantics.- Test updates in
cumulusci/tests/test_utils_clariti.py— ensure mockedsfdxreturns and assertions mirror real wrapper kwargs and fallback behavior.
Possibly related PRs
- DEVOPS-688 feat: add support for clariti org pooling #5 — Modifies the same clariti module and replaces subprocess usage with a wrapper; directly related to these CLI invocation and test changes.
Suggested reviewers
- jsun-clariti
Poem
🐇 I hopped through args and stdout lines,
swapped shells for tidy wrapper signs.
I catch the errors, trim the fluff,
parse JSON fields and pack them tough.
A little rabbit fixed the CLI stuff.
Pre-merge checks and finishing touches
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 21.74% which is insufficient. The required threshold is 80.00%. | You can run @coderabbitai generate docstrings to improve docstring coverage. |
✅ Passed checks (4 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title directly references DEVOPS-735 and clearly summarizes the main fix: improving SF CLI accessibility on Windows for Clariti commands. The change is entirely focused on replacing subprocess calls with the sfdx helper to address this issue. |
| Linked Issues check | ✅ Passed | The pull request implements the coding requirements from DEVOPS-735: replacing direct subprocess.run calls with the sfdx helper from cumulusci.core.sfdx, adding OSError handling for CLI invocation failures, and improving error messages. These changes address the root cause of SF CLI not being accessible. |
| Out of Scope Changes check | ✅ Passed | All changes are directly scoped to fixing the SF CLI accessibility issue: refactoring clariti.py and updating corresponding tests. No unrelated modifications or scope creep detected. |
✨ Finishing touches
- 📝 Generate docstrings
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
- Commit unit tests in branch
fix/DEVOPS-735
📜 Recent review details
Configuration used: Repository: ClaritiSoftware/coderabbit/.coderabbit.yaml
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(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cumulusci/utils/clariti.py
Warning
Review ran into problems
🔥 Problems
Git: Failed to clone repository. Please run the @coderabbitai full review command to re-trigger a full review. If the issue persists, set path_filters to include or exclude specific files.
Comment @coderabbitai help to get the list of available commands and usage tips.
…est_utils_clariti.py
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Repository: ClaritiSoftware/coderabbit/.coderabbit.yaml
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(4 hunks)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cumulusci/tests/test_utils_clariti.py (1)
1-205: Consider adding tests for CLI not found scenario.The PR addresses the issue where "sf CLI not accessible in windows" by adding OSError exception handling to the production code. However, I don't see tests covering the case where the Salesforce CLI is not found or not accessible (FileNotFoundError/OSError).
Consider adding a test case like:
def test_checkout_org_from_pool_handles_cli_not_found(monkeypatch): def fake_sfdx(command, **kwargs): raise FileNotFoundError("sf command not found") monkeypatch.setattr("cumulusci.utils.clariti.sfdx", fake_sfdx) with pytest.raises(ClaritiError) as exc: checkout_org_from_pool("Pool42") message = str(exc.value) assert "Salesforce CLI" in message or "sf" in messageThis would verify that the OSError handling mentioned in the PR summary is working correctly. Should I generate a complete test suite for the CLI not found scenario?
📜 Review details
Configuration used: Repository: ClaritiSoftware/coderabbit/.coderabbit.yaml
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/tests/test_utils_clariti.py(9 hunks)
🔇 Additional comments (2)
cumulusci/tests/test_utils_clariti.py (2)
17-22: Well-structured test helper.The
_make_prochelper correctly creates a mock process result withio.StringIOobjects for stdout/stderr. This aligns with the production code's expectation of file-like objects with.read()methods.Note: If the production code reads from these streams multiple times, it must call
.seek(0)between reads, as StringIO position advances after each read.
47-72: LGTM: Comprehensive test coverage for checkout with alias.The test correctly validates the command string, argument structure, and result parsing. The assertions cover all expected fields in the ClaritiCheckoutResult.
…from_pool and set_sf_alias functions
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
cumulusci/utils/clariti.py (4)
122-125: Consider preserving original exception details in OSError handling.The OSError handler provides a generic message but discards the original exception details that might help diagnose specific Windows path or permission issues.
Apply this diff to preserve exception context:
except OSError as err: - raise ClaritiError("Failed to execute Salesforce CLI 'sf'.") from err + raise ClaritiError(f"Failed to execute Salesforce CLI 'sf': {err}") from err
156-161: Minor: Extraction calls could be slightly more concise.While the current code is functional, the single-tuple wrapping in extraction calls is a bit verbose.
Consider this minor simplification:
- org_id_value = _extract_string(payload, (("orgId",),), allow_missing=True) + org_id_value = _extract_string(payload, [["orgId"]], allow_missing=True) instance_url_value = _extract_string( - payload, (("instanceUrl",),), allow_missing=True + 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_type_value = _extract_string(payload, [["orgType"]], allow_missing=True) + pool_id_value = _extract_string(payload, [["poolId"]], allow_missing=True)This maintains the Sequence[Sequence[str]] type contract while being slightly easier to read.
189-199: LGTM with same OSError note as checkout_org_from_pool.The sfdx integration is correct, and the exception handling pattern matches the other function. The same suggestion about preserving original exception details in the OSError handler applies here as well.
For consistency, consider including the original exception message:
except OSError: - return False, "Failed to execute Salesforce CLI 'sf'." + return False, f"Failed to execute Salesforce CLI 'sf': {err}"Note: You'll need to capture the exception as
except OSError as err:for this change.
127-134: Extract duplicated stdout/stderr logic into a helper function.The stdout/stderr extraction logic (lines 127-134 and 201-208) is identical in both functions. This duplication increases maintenance burden and the risk of inconsistent updates.
Consider extracting to a helper function:
def _extract_sfdx_output(proc) -> Tuple[str, str]: """Extract and decode stdout/stderr from an sfdx process result. :param proc: Process result from sfdx() call. :returns: Tuple of (stdout, stderr) as stripped strings. """ stdout = getattr(proc, "stdout_text", None) stderr = getattr(proc, "stderr_text", None) if stdout is None and getattr(proc, "stdout", None) is not None: stdout = proc.stdout.decode() if isinstance(proc.stdout, bytes) else proc.stdout if stderr is None and getattr(proc, "stderr", None) is not None: stderr = proc.stderr.decode() if isinstance(proc.stderr, bytes) else proc.stderr stdout = stdout.strip() if stdout else "" stderr = stderr.strip() if stderr else "" return stdout, stderrThen replace both occurrences with:
stdout, stderr = _extract_sfdx_output(proc)Also applies to: 201-208
📜 Review details
Configuration used: Repository: ClaritiSoftware/coderabbit/.coderabbit.yaml
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(4 hunks)
🔇 Additional comments (3)
cumulusci/utils/clariti.py (3)
11-11: LGTM: Import added for the sfdx helper.This import is necessary to replace direct subprocess calls with the sfdx wrapper, which should improve cross-platform compatibility (especially Windows).
108-121: LGTM: sfdx helper properly integrated.The command args are correctly constructed as a list, and the
sfdx()call uses appropriate parameters (capture_output=True,check_return=False) to allow custom error handling downstream.
127-134: LGTM: stdout/stderr extraction correctly addresses past review feedback.The extraction logic properly treats
stdout_text/stderr_textas strings (not streams), with appropriate fallback tostdout/stderrand handling of bytes vs. string types. This resolves the previous.read()AttributeError issue.
…rg_from_pool and set_sf_alias functions
|
Note Following test failures are unrelated to changes in this PR: |
2 participants
This pull request refactors the way Salesforce CLI commands are executed in
cumulusci/utils/clariti.pyby replacing directsubprocesscalls with thesfdxhelper fromcumulusci.core.sfdx. This change improves error handling and streamlines command execution, making the codebase more consistent and maintainable.Refactoring command execution
subprocess.runcalls with thesfdxhelper for running Salesforce CLI commands in bothcheckout_org_from_poolandset_sf_aliasfunctions, improving consistency and error handling. [1] [2] [3]Improved error handling
OSErrorexceptions when executing Salesforce CLI commands, providing clearer error messages in both functions. [1] [2]Output processing updates
stdout_textandstderr_textstreams from thesfdxhelper, ensuring output is read and stripped correctly. [1] [2]Minor formatting and code cleanup
_extract_stringby removing unnecessary line breaks for improved readability.Summary by CodeRabbit
Bug Fixes
Chores / Refactor
Tests