Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughAdds kubeconfig-style YAML configuration support and CLI integration: new ConfigFile abstraction for loading/resolving contexts, CLI flags Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as CLI Parser
participant Config as ConfigFile Manager
participant Core as Resolve Defaults
participant Conn as Connection Factory
User->>CLI: sapcli --config /path --context prod <command>
CLI->>CLI: parse args (--config, --context)
CLI->>Config: ConfigFile.load(args.config)
Config-->>CLI: ConfigFile instance (or empty)
CLI->>CLI: determine command transformer
alt command requires no connection
CLI-->>User: return args (skip resolution)
else requires connection
CLI->>Core: resolve_default_connection_values(args)
Core->>Config: resolve_context(args.context or SAPCLI_CONTEXT or current-context)
Config-->>Core: flattened context values
Core->>Core: apply precedence (CLI > ENV > Config > defaults)
Core->>Conn: build connection with resolved values
Conn-->>CLI: connection object
CLI-->>User: execute command with connection
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 3
🧹 Nitpick comments (9)
doc/configuration.md (3)
98-100: Add language specifier to fenced code block.The fenced code block should specify a language for consistency and proper syntax highlighting.
Suggested fix
-``` +```bash sapcli --config /path/to/custom-config.yml program read ZREPORT</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@doc/configuration.mdaround lines 98 - 100, Update the fenced code block
that contains the command "sapcli --config /path/to/custom-config.yml program
read ZREPORT" to include a language specifier (use "bash") so the block starts
with ```bash for proper syntax highlighting; locate the exact fenced block
around that command and change the opening fence accordingly.</details> --- `108-110`: **Add language specifier to fenced code block.** Same as above - specify a language for the code block. <details> <summary>Suggested fix</summary> ```diff -``` +```bash sapcli --context prod program read ZREPORT ``` ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@doc/configuration.md` around lines 108 - 110, The fenced code block containing the command "sapcli --context prod program read ZREPORT" is missing a language specifier; update that markdown block to include a language tag (e.g., bash) so syntax highlighting is applied—for the fenced block showing sapcli --context prod program read ZREPORT, change the opening backticks to include the language specifier (bash). ``` </details> --- `256-268`: **Add language specifier to fenced code block.** The config management commands example should specify a language for consistency. <details> <summary>Suggested fix</summary> ```diff -``` +```bash # Show the current effective configuration sapcli config view # Show the current context name sapcli config current-context # Switch the active context sapcli config use-context prod # List available contexts sapcli config get-contexts ``` ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@doc/configuration.mdaround lines 256 - 268, The fenced code block showing
config management examples lacks a language specifier; update the block
containing the "sapcli config view", "sapcli config current-context", "sapcli
config use-context prod", and "sapcli config get-contexts" examples to use a
bash language tag (e.g., replace the openingwithbash) so syntax
highlighting and consistency are applied across documentation.</details> </blockquote></details> <details> <summary>test/unit/test_sap_cli_config.py (1)</summary><blockquote> `12-13`: **Remove unused imports.** `PrintConsole` and `ConfigFile` are imported but never used in this test module. <details> <summary>Suggested fix</summary> ```diff import sap.cli.config -from sap.cli.core import PrintConsole -from sap.config import ConfigFile ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@test/unit/test_sap_cli_config.py` around lines 12 - 13, The test module imports unused symbols PrintConsole and ConfigFile from sap.cli.core and sap.config respectively; remove these unused imports from test_sap_cli_config.py by deleting PrintConsole and ConfigFile from the import statements (or removing the entire import lines if they only import those names) so the file no longer includes unused references to PrintConsole and ConfigFile. ``` </details> </blockquote></details> <details> <summary>sap/config.py (2)</summary><blockquote> `253-261`: **Redundant filter condition.** The check `field not in ('connection', 'user')` is unnecessary because `'connection'` and `'user'` are not members of `CONTEXT_OVERRIDE_FIELDS` (which only contains `CONNECTION_FIELDS + USER_FIELDS + CONTEXT_FIELDS`). <details> <summary>♻️ Suggested simplification</summary> ```diff # Context-level overrides (any connection, user, or context field # specified directly on the context definition wins). - # Skip 'connection' and 'user' as these are structural reference - # keys in the context, not override values. for field in CONTEXT_OVERRIDE_FIELDS: - if field not in ('connection', 'user') and field in context: + if field in context: result[field] = context[field] ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@sap/config.py` around lines 253 - 261, The loop over CONTEXT_OVERRIDE_FIELDS contains a redundant check for "field not in ('connection', 'user')" because CONTEXT_OVERRIDE_FIELDS is built from CONNECTION_FIELDS + USER_FIELDS + CONTEXT_FIELDS and never includes those string keys; in the block that iterates CONTEXT_OVERRIDE_FIELDS (referencing CONTEXT_OVERRIDE_FIELDS, result, and context), remove the unnecessary exclusion and simplify the condition to only test membership in context before assigning result[field] = context[field]. ``` </details> --- `263-275`: **New config files may be created with insecure permissions.** When `save()` creates a new config file, it uses default umask permissions which may be world-readable. If the user later adds passwords, the file would already have permissive permissions. Consider setting restrictive permissions (0600) on newly created files. <details> <summary>🔒 Suggested improvement for secure file creation</summary> ```diff save_path.parent.mkdir(parents=True, exist_ok=True) - with open(save_path, 'w', encoding='utf-8') as config_file: - yaml.dump(self._data, config_file, default_flow_style=False, sort_keys=False) + # Create with restrictive permissions (owner read/write only) + fd = os.open(save_path, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600) + try: + with os.fdopen(fd, 'w', encoding='utf-8') as config_file: + yaml.dump(self._data, config_file, default_flow_style=False, sort_keys=False) + except: + os.close(fd) + raise self._path = save_path ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@sap/config.py` around lines 263 - 275, The save() method currently writes config files with default umask, possibly leaving them world-readable; update save() to enforce restrictive permissions (0600) on newly created files by checking whether save_path.exists() before writing and, after successfully writing yaml, call save_path.chmod(0o600) only when the file was not present previously; reference the save() method, save_path and self._path (and DEFAULT_CONFIG_PATH) when locating where to add the existence check and the Path.chmod(0o600) call. ``` </details> </blockquote></details> <details> <summary>sap/cli/__init__.py (2)</summary><blockquote> `281-298`: **Unused environment variable names in mapping.** The `extra_params` dict maps parameter names to environment variable names, but only the parameter names (keys) are used. If env var fallback isn't needed here (it's handled elsewhere), simplify to a tuple. <details> <summary>♻️ Suggested simplification</summary> ```diff - extra_params = { - 'mshost': 'SAP_MSHOST', - 'msserv': 'SAP_MSSERV', - 'sysid': 'SAP_SYSID', - 'group': 'SAP_GROUP', - 'snc_qop': 'SNC_QOP', - 'snc_myname': 'SNC_MYNAME', - 'snc_partnername': 'SNC_PARTNERNAME', - 'snc_lib': 'SNC_LIB', - } - - for param in extra_params: + extra_params = ( + 'mshost', 'msserv', 'sysid', 'group', + 'snc_qop', 'snc_myname', 'snc_partnername', 'snc_lib', + ) + + for param in extra_params: if hasattr(args, param) and not getattr(args, param) and param in config_values: setattr(args, param, config_values[param]) ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@sap/cli/__init__.py` around lines 281 - 298, The extra_params mapping in _apply_config_extra_params contains unused environment variable names as values; replace it with a simple iterable of parameter names (e.g., tuple or list of 'mshost', 'msserv', 'sysid', 'group', 'snc_qop', 'snc_myname', 'snc_partnername', 'snc_lib') and iterate over that, keeping the same conditional logic that checks hasattr(args, param), not getattr(args, param), and param in config_values before calling setattr(args, param, config_values[param]); remove the unused mapping values to simplify the code. ``` </details> --- `273-276`: **Consider logging a warning when config loading fails.** Silently returning an empty dict on `SAPCliConfigError` may hide misconfiguration from users (e.g., typos in context names). A debug/warning log would help troubleshooting while still allowing fallback to env vars. <details> <summary>💡 Suggested improvement</summary> ```diff + import logging + try: result = config_file.resolve_context(context_name) except SAPCliConfigError: + logging.debug('Failed to resolve config context, falling back to env vars') return {} ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@sap/cli/__init__.py` around lines 273 - 276, When resolve_context fails, don't swallow SAPCliConfigError silently; update the except block around config_file.resolve_context(context_name) to catch the exception as e and emit a warning-level log (e.g., logger.warning) that includes the failing context_name and the exception message before returning {} so users see a helpful hint about misconfiguration while preserving the env-var fallback. ``` </details> </blockquote></details> <details> <summary>test/test_packaging.py (1)</summary><blockquote> `33-35`: **Prefer `unittest` assertions over bare `assert` in `TestCase` methods.** Use framework-native assertions for clearer failure output and consistent test behavior. <details> <summary>Proposed refactor</summary> ```diff - assert backend == "setuptools.build_meta", ( - f"build-backend should be 'setuptools.build_meta', got '{backend}'" - ) + self.assertEqual( + backend, + "setuptools.build_meta", + f"build-backend should be 'setuptools.build_meta', got '{backend}'", + ) ... - assert any("setuptools" in r for r in requires), ( - "build-system.requires should include setuptools" - ) + self.assertTrue( + any("setuptools" in r for r in requires), + "build-system.requires should include setuptools", + ) ``` </details> Also applies to: 49-51 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@test/test_packaging.py` around lines 33 - 35, Replace bare assert statements in TestCase methods with unittest TestCase assertion helpers: change the assertion checking backend ("assert backend == 'setuptools.build_meta', ...") to use self.assertEqual(backend, "setuptools.build_meta", msg=...) so failures produce framework-native output; do the same for the other similar assertion at lines 49-51. Locate these in test_packaging.py inside the TestCase class (the method that obtains `backend`) and replace bare asserts with self.assertEqual (include the existing failure message as the msg parameter). ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Inline comments:
In@test/test_packaging.py:
- Around line 31-32: The tests in test_packaging.py assign the result of
load_pyproject() to the misspelled variable name "pyroject" but subsequently
read "pyproject", causing NameError failures; update each assignment to use the
correct variable name "pyproject" so the value returned by load_pyproject() is
read consistently (fix the occurrences where "pyroject = load_pyproject()" to
"pyproject = load_pyproject()"; occurrences also appear in the other two test
blocks referenced).- Around line 37-44: The test test_build_backend_is_importable uses an undefined
variable name "pyroject" and calls pytest.fail() inside a unittest.TestCase
method; change the typo so you call load_pyproject() result into the correct
variable name pyproject and replace pytest.fail(...) with self.fail(...). Locate
the function test_build_backend_is_importable and all other tests in this file
that use "pyroject" (e.g., other test methods around the same area), correct the
variable name to pyproject where load_pyproject() is assigned, and use self.fail
to signal test failure instead of pytest.fail so the unittest.TestCase API is
used consistently.In
@test/unit/test_sap_config.py:
- Around line 113-122: The test test_default_path_not_exists assigns result from
_resolve_config_path() but has no assertion; update the test to either remove it
or make it deterministic by controlling the home directory and asserting
expected behavior: e.g., in test_default_path_not_exists patch HOME to a temp
directory with no ~/.sapcli/config.yml, call _resolve_config_path() and assert
the result is None (or whatever the intended return is), referencing the test
function name test_default_path_not_exists and the helper function
_resolve_config_path to locate and modify the test.
Nitpick comments:
In@doc/configuration.md:
- Around line 98-100: Update the fenced code block that contains the command
"sapcli --config /path/to/custom-config.yml program read ZREPORT" to include a
language specifier (use "bash") so the block starts with ```bash for proper
syntax highlighting; locate the exact fenced block around that command and
change the opening fence accordingly.- Around line 108-110: The fenced code block containing the command "sapcli
--context prod program read ZREPORT" is missing a language specifier; update
that markdown block to include a language tag (e.g., bash) so syntax
highlighting is applied—for the fenced block showing sapcli --context prod
program read ZREPORT, change the opening backticks to include the language
specifier (bash).- Around line 256-268: The fenced code block showing config management examples
lacks a language specifier; update the block containing the "sapcli config
view", "sapcli config current-context", "sapcli config use-context prod", and
"sapcli config get-contexts" examples to use a bash language tag (e.g., replace
the openingwithbash) so syntax highlighting and consistency are applied
across documentation.In
@sap/cli/__init__.py:
- Around line 281-298: The extra_params mapping in _apply_config_extra_params
contains unused environment variable names as values; replace it with a simple
iterable of parameter names (e.g., tuple or list of 'mshost', 'msserv', 'sysid',
'group', 'snc_qop', 'snc_myname', 'snc_partnername', 'snc_lib') and iterate over
that, keeping the same conditional logic that checks hasattr(args, param), not
getattr(args, param), and param in config_values before calling setattr(args,
param, config_values[param]); remove the unused mapping values to simplify the
code.- Around line 273-276: When resolve_context fails, don't swallow
SAPCliConfigError silently; update the except block around
config_file.resolve_context(context_name) to catch the exception as e and emit a
warning-level log (e.g., logger.warning) that includes the failing context_name
and the exception message before returning {} so users see a helpful hint about
misconfiguration while preserving the env-var fallback.In
@sap/config.py:
- Around line 253-261: The loop over CONTEXT_OVERRIDE_FIELDS contains a
redundant check for "field not in ('connection', 'user')" because
CONTEXT_OVERRIDE_FIELDS is built from CONNECTION_FIELDS + USER_FIELDS +
CONTEXT_FIELDS and never includes those string keys; in the block that iterates
CONTEXT_OVERRIDE_FIELDS (referencing CONTEXT_OVERRIDE_FIELDS, result, and
context), remove the unnecessary exclusion and simplify the condition to only
test membership in context before assigning result[field] = context[field].- Around line 263-275: The save() method currently writes config files with
default umask, possibly leaving them world-readable; update save() to enforce
restrictive permissions (0600) on newly created files by checking whether
save_path.exists() before writing and, after successfully writing yaml, call
save_path.chmod(0o600) only when the file was not present previously; reference
the save() method, save_path and self._path (and DEFAULT_CONFIG_PATH) when
locating where to add the existence check and the Path.chmod(0o600) call.In
@test/test_packaging.py:
- Around line 33-35: Replace bare assert statements in TestCase methods with
unittest TestCase assertion helpers: change the assertion checking backend
("assert backend == 'setuptools.build_meta', ...") to use
self.assertEqual(backend, "setuptools.build_meta", msg=...) so failures produce
framework-native output; do the same for the other similar assertion at lines
49-51. Locate these in test_packaging.py inside the TestCase class (the method
that obtainsbackend) and replace bare asserts with self.assertEqual (include
the existing failure message as the msg parameter).In
@test/unit/test_sap_cli_config.py:
- Around line 12-13: The test module imports unused symbols PrintConsole and
ConfigFile from sap.cli.core and sap.config respectively; remove these unused
imports from test_sap_cli_config.py by deleting PrintConsole and ConfigFile from
the import statements (or removing the entire import lines if they only import
those names) so the file no longer includes unused references to PrintConsole
and ConfigFile.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Organization UI **Review profile**: CHILL **Plan**: Pro **Run ID**: `34e37850-886c-46cb-8d31-39549197080d` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 4ce6c12ac43bb37d293a513393637ae34f7ce898 and 2024a400aaace617244c9a13fe4eb57a5945cbd2. </details> <details> <summary>📒 Files selected for processing (11)</summary> * `README.md` * `doc/configuration.md` * `sap/cli/__init__.py` * `sap/cli/_entry.py` * `sap/cli/config.py` * `sap/config.py` * `test/test_packaging.py` * `test/unit/test_sap_cli.py` * `test/unit/test_sap_cli__entry.py` * `test/unit/test_sap_cli_config.py` * `test/unit/test_sap_config.py` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
filak-sap
force-pushed
the
configuration_file
branch
from
2024a40 to
bf2ac1c
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@doc/configuration.md`:
- Around line 98-100: The markdown fenced code blocks with sapcli examples
(e.g., the block containing "sapcli --config /path/to/custom-config.yml program
read ZREPORT" and other blocks referencing "sapcli --context prod ..." and the
multi-line block with "sapcli config view", "sapcli config current-context",
"sapcli config use-context prod", "sapcli config get-contexts") are missing
language hints and trigger MD040; update each triple-backtick fence to include
the bash language (```bash) so the examples are correctly tagged for
markdownlint and syntax highlighting.
- Around line 226-228: Update the wording to clarify that in a context the
`user` field is a reference key to an entry in `users.<name>` (handled by
sap/config.py) and is not itself overridden by inline context values; only other
user fields (e.g., `password`) provided on the context will override the
referenced user's corresponding fields. Replace the ambiguous sentence that says
"a context can contain any field from `users.<name>`" with a statement that
`user` remains a reference and show a corrected example (e.g.,
`contexts.prod.user: DEPLOYER` keeps the username from the `DEPLOYER` user
entry, while `contexts.prod.password: ...` would override the password). Ensure
the doc mentions `sap/config.py` behavior so readers know `user` is treated as a
name lookup, not an overridable username.
In `@sap/config.py`:
- Around line 98-109: The _has_passwords function currently only inspects users
and misses inline context-level password overrides used by resolve_context;
update _has_passwords to also examine config_data.get('contexts', {}) and return
True if any context definition (when isinstance(dict)) contains a truthy
'password' key (similar to the existing users loop). Keep the existing users
check and add the contexts scan (checking dict types and 'password' presence) so
configs that store secrets under contexts.*.password won't suppress the
permission warning.
- Around line 81-95: _wrap YAML parsing errors in SAPCliConfigError in
_load_config_file: catch yaml.YAMLError (and subclasses like ParserError) around
yaml.safe_load and raise SAPCliConfigError with a helpful message instead of
letting YAMLError propagate. In get_context, get_connection, get_user and
resolve_context, validate that retrieved items are dicts before treating them
like mappings (e.g., after fetching data['contexts'] or the
context/connection/user object); if a value is missing or not a dict, raise
SAPCliConfigError with a clear message about malformed config. Ensure all raised
errors use SAPCliConfigError so malformed top-level or nested YAML shapes
produce uniform, user-facing errors.
In `@test/unit/test_sap_cli_config.py`:
- Around line 12-13: Remove the unused imports PrintConsole and ConfigFile from
the test module to satisfy linting: locate the import statement that currently
reads "from sap.cli.core import PrintConsole" and "from sap.config import
ConfigFile" in test_sap_cli_config.py and delete those names (or remove the
entire import lines if nothing else is imported from those modules) so the file
no longer imports PrintConsole or ConfigFile.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 174fd93a-5282-4828-ae29-dce562fb1e90
📒 Files selected for processing (10)
README.mddoc/configuration.mdsap/cli/__init__.pysap/cli/_entry.pysap/cli/config.pysap/config.pytest/unit/test_sap_cli.pytest/unit/test_sap_cli__entry.pytest/unit/test_sap_cli_config.pytest/unit/test_sap_config.py
✅ Files skipped from review due to trivial changes (2)
- README.md
- sap/cli/config.py
🚧 Files skipped from review as they are similar to previous changes (2)
- test/unit/test_sap_cli.py
- sap/cli/init.py
filak-sap
force-pushed
the
configuration_file
branch
from
bf2ac1c to
c1a9206
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@sap/cli/__init__.py`:
- Around line 234-245: The code uses bool(config_values['ssl']) and
bool(config_values['ssl_verify']) which treat quoted YAML strings like "false"
as True; update the logic around args.ssl and args.verify to detect non-boolean
scalars and normalize string false tokens the same way as the env-var branch
(e.g., if the config value is a bool use it directly, if it's a string compare
value.lower() against ('n','no','false','off') to set False otherwise True, or
treat unexpected types as an error/default), specifically adjust the branches
that set args.ssl (checking config_values['ssl']) and args.verify (checking
config_values['ssl_verify']) to perform this normalization instead of calling
bool() on the raw value.
- Around line 273-276: The current except block swallowing SAPCliConfigError
after calling resolve_context(context_name) leads to silently using defaults;
change the handler so failures are not converted to an empty dict — either
remove the except so the SAPCliConfigError propagates, or re-raise a new,
clearer error that includes the context_name (e.g., raise
SAPCliConfigError(f"Invalid context '{context_name}': {err}") ) instead of
returning {} to ensure misspelled or broken contexts fail fast; update the code
around resolve_context(context_name) and the SAPCliConfigError handling
accordingly.
In `@sap/config.py`:
- Around line 84-90: The code only converts YAML parse errors to
SAPCliConfigError but not file I/O errors; wrap the open(path, ...) call in a
try/except that catches OSError/PermissionError (or broadly OSError) around
opening the file and raise SAPCliConfigError with a clear message that includes
path and the original exception, preserving the original exception as the cause
(from ex); keep the existing yaml.safe_load try/except intact so yaml.YAMLError
still maps to SAPCliConfigError and reference the same symbols: path, open(...),
config_file, yaml.safe_load, and SAPCliConfigError.
- Around line 317-318: The current save logic uses open(save_path, 'w') which
honors the process umask and can create world-readable files; change the save to
create the file with restrictive permissions (e.g. 0o600) before writing: use
os.open(save_path, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600) then wrap the
returned fd with os.fdopen(fd, 'w', encoding='utf-8') and call
yaml.dump(self._data, config_file, default_flow_style=False, sort_keys=False);
ensure os is imported and preserve the same save_path and self._data symbols so
the file is created with owner-read/write only.
- Around line 134-136: The code currently calls
_resolve_config_path(cli_config_path) and when it returns None it returns
ConfigFile({}) which drops the caller-supplied path; instead, when
_resolve_config_path returns None you should construct the ConfigFile using the
original requested path (the cli_config_path / env-provided path) so that later
save() uses that explicit path; update the branch in the function that checks
path is None to pass the caller-supplied path into ConfigFile (or otherwise
record it on the ConfigFile instance) and keep existing behavior when a resolved
path is returned.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6c7f21aa-01b7-4d0e-a5d8-fcb9abf591f4
📒 Files selected for processing (10)
README.mddoc/configuration.mdsap/cli/__init__.pysap/cli/_entry.pysap/cli/config.pysap/config.pytest/unit/test_sap_cli.pytest/unit/test_sap_cli__entry.pytest/unit/test_sap_cli_config.pytest/unit/test_sap_config.py
✅ Files skipped from review due to trivial changes (3)
- doc/configuration.md
- test/unit/test_sap_cli_config.py
- README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- sap/cli/config.py
filak-sap
force-pushed
the
configuration_file
branch
from
c1a9206 to
33847ea
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@sap/cli/__init__.py`:
- Around line 239-245: The code currently calls int() on untrusted inputs when
setting args.port (os.getenv('SAP_PORT') and config_values['port']) which can
raise ValueError; update the logic in the args.port assignment block to validate
and convert both sources inside try/except blocks and on ValueError raise
SAPCliConfigError with a clear message (e.g., include the offending value and
that it must be an integer). Specifically, wrap the int(os.getenv('SAP_PORT'))
conversion and the int(config_values['port']) conversion in separate try/except
ValueError handlers and raise SAPCliConfigError from the except so callers get a
user-friendly error; keep references to args.port, os.getenv('SAP_PORT'),
config_values['port'], and SAPCliConfigError to locate the changes.
In `@sap/config.py`:
- Around line 21-26: CONNECTION_FIELDS currently advertises 'http_timeout' but
it's never applied; remove 'http_timeout' from the CONNECTION_FIELDS tuple (or
alternatively wire it end-to-end) so config doesn't silently advertise an unused
option — either delete 'http_timeout' from CONNECTION_FIELDS or update
resolve_default_connection_values() to copy 'http_timeout' and update
config_get() to read/apply SAPCLI_HTTP_TIMEOUT (referencing the symbols
CONNECTION_FIELDS, resolve_default_connection_values(), and config_get() when
making the change).
- Around line 134-136: The code currently treats any non-file path as "no
config" and silently returns ConfigFile({}, path); change _resolve_config_path
handling so that if a user supplied an explicit config path and the resolved
path exists but is not a regular file (e.g., is a directory), the code raises a
clear error (ValueError or SystemExit) instead of returning an empty ConfigFile.
In practice, inside the logic around _resolve_config_path and the creation of
ConfigFile, test path.exists() and path.is_file(): if path.exists() and not
path.is_file(), raise an informative exception indicating the provided --config
is not a file; keep the existing fallback behavior only for non-existent paths.
- Around line 311-318: The save() method currently performs filesystem and YAML
writes (mkdir, os.open, yaml.dump) without error handling; wrap the block that
creates save_path.parent.mkdir(...), opens the fd with os.open(...), and writes
via yaml.dump(...) in a try/except that catches OSError and yaml.YAMLError (or
Exception) and re-raises a SAPCliConfigError with a clear message and the
original exception attached (e.g., using exception chaining) so callers get a
handled error; ensure self._path is only updated after the write succeeds and
keep existing file mode 0o600 behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 763f775b-17a3-496d-81e1-6dcb4d095224
📒 Files selected for processing (10)
README.mddoc/configuration.mdsap/cli/__init__.pysap/cli/_entry.pysap/cli/config.pysap/config.pytest/unit/test_sap_cli.pytest/unit/test_sap_cli__entry.pytest/unit/test_sap_cli_config.pytest/unit/test_sap_config.py
✅ Files skipped from review due to trivial changes (3)
- README.md
- sap/cli/config.py
- test/unit/test_sap_cli_config.py
🚧 Files skipped from review as they are similar to previous changes (3)
- doc/configuration.md
- test/unit/test_sap_cli__entry.py
- test/unit/test_sap_cli.py
I am not gonna lie to you, I just wrote basic details of how I picture the configuration should work and then I asked Opus 4.6 to check how other projects like kubectl solves it. Eventually we decided to implement the most similar way to kubectl, so users do not need to learn much. The feature spec was like this: sapcli is currently configured only via environment variables. Users can easily set them but it is not very user-friendly. Environment variables work fine in pipelines. Considered other options: - Add kubectl config style with connection (instead of server), user and context. We have many systems with equal configuration except the hostname - hence we a context specification must support overriding field from its referenced connection or user definition. - Reuse SAPGUI `~/.SAPGUI/SAPGUILandscape.xml`: Could serve as a read-only source of connection definitions for desktop users. Does not contain credentials. Could be supported as an optional import/fallback, but should not be the primary config mechanism. - Reuse ADT Eclipse workspace settings: Eclipse workspace settings are tightly coupled to the Eclipse workspace structure and would be fragile to parse. Deprioritized.
filak-sap
force-pushed
the
configuration_file
branch
from
33847ea to
2efdd13
Compare
This was referenced Mar 27, 2026
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
2 participants
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.
I am not gonna lie to you, I just wrote basic details of how I picture the configuration should work and then I asked Opus 4.6 to check how other projects like kubectl solves it.
Eventually we decided to implement the most similar way to kubectl, so users do not need to learn much.
The feature spec was like this:
sapcli is currently configured only via environment variables. Users can easily set them but it is not very user-friendly. Environment variables work fine in pipelines.
Considered other options:
Add kubectl config style with connection (instead of server), user and context. We have many systems with equal configuration except the hostname - hence we a context specification must support overriding field from its referenced connection or user definition.
Reuse SAPGUI
~/.SAPGUI/SAPGUILandscape.xml: Could serve as a read-only source of connection definitions for desktop users. Does not contain credentials. Could be supported as an optional import/fallback, but should not be the primary config mechanism.Reuse ADT Eclipse workspace settings: Eclipse workspace settings are tightly coupled to the Eclipse workspace structure and would be fragile to parse. Deprioritized.