feat(no-ticket): add cloudsmith repos privileges command group - #389
Closed
BartoszBlizniak wants to merge 1 commit into
Closed
feat(no-ticket): add cloudsmith repos privileges command group#389BartoszBlizniak wants to merge 1 commit into
cloudsmith repos privileges command group#389BartoszBlizniak wants to merge 1 commit into
Conversation
Adds list/set/revoke/replace for the explicit team, user and service account privileges on a repository. Only revoke and replace can take access away, so only those two confirm before writing. The API returns every privilege in one response and ignores page parameters, so list offers no page options. It also has no way to delete a single privilege, so revoke reads the current set and writes back what is being kept. confirm_operation() now accepts an explicit empty prefix so these commands can ask their question directly, and handle_api_exceptions() takes an optional summarise_error callable so the API's field-indexed 422 reads as one sentence naming the repository. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
- Adds a new
cloudsmith repos privilegescommand group to manage explicit repository access (teams/users/service accounts) via list/set/revoke/replace workflows, aligned with the API’s whole-list semantics and lack of single-delete. - Improves CLI UX for destructive operations and API error rendering to better support scripting and clearer human output.
Changes:
- Introduces new core API wrappers for repository privileges (
list,patch,put) plus focused unit tests. - Adds
repos privilegesCLI commands (with JSON-friendly output behavior and confirmation for destructive operations). - Extends shared CLI utilities: optional empty confirmation prefix and optional one-line API error summarisation.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| cloudsmith_cli/core/tests/test_repos_privileges.py | Adds core-level tests for privileges list/update/replace API wrappers. |
| cloudsmith_cli/core/api/repos.py | Adds list_repo_privileges, update_repo_privileges, and replace_repo_privileges wrappers around SDK calls. |
| cloudsmith_cli/cli/utils.py | Allows confirm_operation() to accept an explicit empty prefix (ask question directly). |
| cloudsmith_cli/cli/tests/test_utils.py | Adds tests covering the new confirm_operation() prefix behavior. |
| cloudsmith_cli/cli/tests/test_exceptions.py | Adds tests for the new one-sentence API error summarisation hook. |
| cloudsmith_cli/cli/tests/commands/test_repos_privileges.py | Adds CLI command tests for list/set/revoke/replace behaviors and JSON/stderr semantics. |
| cloudsmith_cli/cli/exceptions.py | Adds summarise_error hook to handle_api_exceptions() to optionally replace verbose 422 field blocks. |
| cloudsmith_cli/cli/commands/repos.py | Implements the new repos privileges group and subcommands, including input validation and output rendering. |
| CHANGELOG.md | Documents the new cloudsmith repos privileges feature under Unreleased. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+409
to
+416
| for name in names: | ||
| if not name.strip(): | ||
| raise click.UsageError(f"Specify a slug for --{kind}.") | ||
|
|
||
| if (kind, name) in seen: | ||
| raise click.UsageError(f"Specified more than once: {kind} {name}.") | ||
| seen.add((kind, name)) | ||
| targets.append((kind, name)) |
Comment on lines
40
to
+43
| context_msg = context_msg or "Failed to perform operation!" | ||
| detail, fields = get_details(exc) | ||
| hint = get_error_hint(ctx, opts, exc) | ||
| summary = summarise_error(exc, detail, fields) if summarise_error else None |
Member
Author
|
Superseded by #393 - the head branch was renamed to |
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.
Description
Adds a
cloudsmith repos privilegescommand group for managing who has explicit access to a repository: teams, users and service accounts, all treated the same way.Four subcommands, split by what they can take away:
listsetrevokereplaceOnly
revokeandreplacecan remove access, so only those two confirm before writing. Both take-y.Three API constraints shaped the design, and are worth knowing before reviewing:
listdeliberately offers no page options rather than advertising paging that never happens.revokereads the current set and writes back what is being kept. A concurrent change by someone else can therefore be lost, which the command's help says out loud.Type | Name | Privilegeinstead of a sparse column per kind, and why bothsetandreplacereject an entry that names more than one.Two things outside the command module:
cli/utils.pyconfirm_operation()now accepts an explicit emptyprefixso a command can ask its question directly instead of through the shared "Are you absolutely certain you want to..." preamble. Existing callers pass no prefix and are unaffected.cli/exceptions.pyhandle_api_exceptions()takes an optionalsummarise_errorcallable, which receives the exception and can decline by returningNone. The privileges commands use it to turn the API's field-indexed 422 into one sentence and decline everything else, so a 403, a 404, or a 422 the API declines to explain all keep their status code and hint. Every other caller keeps the existing block unchanged.Type of Change
Additional Notes
Exercised end to end against a live organisation on a scratch repository, which is where the one real bug turned up: a listed privilege carries an explicit
nullfor the two target kinds that do not apply, and the PUT endpoint rejects those nulls outright, sorevokefailed on its write-back. Writes now send only the key that applies. Unit tests cover that shape directly, since a mocked response that omits the nulls would not have caught it.Also verified by hand, because the unit tests only assert the shape and not the API's actual behaviour:
setreally is an upsert (naming one team leaves every other privilege alone), runningrevoketwice is a no-op that still exits 0, andlist -F json | jq '.data' | replace - -yround-trips because listing and replacing share an entry shape.setdoes not report that a named target already had the privilege it was given. The PATCH returns 204 with no after-state, so saying "already had this" would mean a GET before every write to produce an advisory line nobody acts on. It prints the entries it wrote rather than the repository's resulting state;listis there for that.Two edges worth a reviewer's attention, both found by running this against a live org rather than by the tests:
replacewith a file that lists nothing revokes every explicit privilege, and that includes the caller's own. On a private repository that means losing sight of it entirely, so the confirmation for an empty file drops the "replace all 0 privileges" phrasing and asksRevoke all explicit access to your-repo in the your-org namespace?instead.replace your-org/your-repo -reads the document from stdin, which leaves nothing for the confirmation to read an answer from. It now refuses up front and asks for-yrather than aborting with no explanation.revokehas to write the whole list back, so every privilege it keeps has to be one it can express. If it reads an entry it cannot (no principal, or a kind a future API adds), it refuses and points atreplacerather than silently revoking it as a side effect. Declining either confirmation writes nothing and prints nothing, including under-F json; the paths that run to completion always emit a document.