Merged
Conversation
Add 8 new CLI subcommands to manage the sapcli configuration file: set-connection, delete-connection, get-connections, set-user, delete-user, get-users, set-context, and delete-context. Design decisions: Naming follows kubectl/git conventions (set-*, delete-*, get-*). The set-* commands use upsert semantics: they create the entry if it does not exist, or merge the provided fields into the existing entry if it does. This avoids the need for separate add/update commands and keeps the CLI surface minimal. Merge/patch behavior means that when updating an existing entry, only the fields explicitly passed on the command line are changed; all other fields are preserved. This is achieved by collecting only non-None argument values and passing them to dict.update(). Boolean fields like --ssl need paired --ssl/--no-ssl flags with default=None so we can distinguish 'not specified' from an explicit True or False. Referential integrity on delete: delete-connection and delete-user refuse to proceed if any context references the named entry, unless --force is passed. This prevents dangling references that would cause confusing errors at runtime. delete-context has no such constraint since contexts are leaf nodes in the reference graph. Deleting the current-context automatically unsets current-context. Each field has its own --flag rather than key=value pairs, which provides better discoverability via --help, shell completion, and type validation (e.g. --port is typed as int). The --user flag in set-user uses dest='user_value' and in set-context uses dest='user_ref' to avoid collisions with the global --user argument that argparse injects into the namespace. Similarly, set-context uses dest='ctx_password' for --password. All new ConfigFile methods (_ensure_section, set_connection, set_user, set_context, find_referencing_contexts, delete_connection, delete_user, delete_context) are added to the model layer in sap/config.py, keeping the CLI layer thin: commands just parse arguments, delegate to ConfigFile, call save(), and print feedback. Error handling uses SAPCliConfigError throughout, which the CLI entry point already intercepts to print a user-friendly message instead of a stack trace. Also fixes a pre-existing mypy error by adding a type annotation to the summary variable in merge_into().
|
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 (5)
📝 WalkthroughWalkthroughThis PR extends the sapcli configuration subsystem with CRUD operations for managing connections, users, and contexts. It introduces CLI commands ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
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
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.
Add 8 new CLI subcommands to manage the sapcli configuration file: set-connection, delete-connection, get-connections, set-user, delete-user, get-users, set-context, and delete-context.
Design decisions:
Naming follows kubectl/git conventions (set-, delete-, get-). The set- commands use upsert semantics: they create the entry if it does not exist, or merge the provided fields into the existing entry if it does. This avoids the need for separate add/update commands and keeps the CLI surface minimal.
Merge/patch behavior means that when updating an existing entry, only the fields explicitly passed on the command line are changed; all other fields are preserved. This is achieved by collecting only non-None argument values and passing them to dict.update(). Boolean fields like --ssl need paired --ssl/--no-ssl flags with default=None so we can distinguish 'not specified' from an explicit True or False.
Referential integrity on delete: delete-connection and delete-user refuse to proceed if any context references the named entry, unless --force is passed. This prevents dangling references that would cause confusing errors at runtime. delete-context has no such constraint since contexts are leaf nodes in the reference graph. Deleting the current-context automatically unsets current-context.
Each field has its own --flag rather than key=value pairs, which provides better discoverability via --help, shell completion, and type validation (e.g. --port is typed as int).
The --user flag in set-user uses dest='user_value' and in set-context uses dest='user_ref' to avoid collisions with the global --user argument that argparse injects into the namespace. Similarly, set-context uses dest='ctx_password' for --password.
All new ConfigFile methods (_ensure_section, set_connection, set_user, set_context, find_referencing_contexts, delete_connection, delete_user, delete_context) are added to the model layer in sap/config.py, keeping the CLI layer thin: commands just parse arguments, delegate to ConfigFile, call save(), and print feedback.
Error handling uses SAPCliConfigError throughout, which the CLI entry point already intercepts to print a user-friendly message instead of a stack trace.
Also fixes a pre-existing mypy error by adding a type annotation to the summary variable in merge_into().