Fix review issues in migrate-users / create-workspaces PR#21
Merged
dsblank merged 2 commits intochasefortier/migrate-workspaces-and-usersfrom Mar 25, 2026
Merged
Conversation
d609047 to
7cd5e4b
Compare
- copy.py: move `import requests` to top-level; use `self.api._get_url_server()` instead of broken `self.api.server_url`; split HTTPError vs RequestException handlers for precise error messages - migrate_users.py: add --url/--source-url args for self-hosted instances; fix _resolve_server_url (base64 padding, error handling, explicit-URL priority); require --source-api-key explicitly instead of silently reusing COMET_API_KEY; warn when source and dest URL+key are identical; handle dict-list API response in _get_existing_workspaces; show per-user output in dry-run; add --failures-output flag; use json= kwarg in _add_member; fix docstring examples - tests/unit/test_migrate_users.py: 17 new unit tests covering _resolve_server_url, _get_existing_workspaces, _add_member, and dry-run behaviour Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
b2a1e72 to
125a3e2
Compare
- Fix duplicate migrate-users subparser registration in __init__.py
- Validate explicit URLs require https:// in _resolve_server_url (SSRF fix)
- Remove silent COMET_CLOUD_URL fallback; fail fast when URL cannot be determined
- Handle {"workspaceNames": [...]} dict response shape in _get_existing_workspaces
- Deduplicate workspace creation by reusing _create_workspace from migrate_users in copy.py
- Update tests to match new behavior (SystemExit instead of COMET_CLOUD_URL fallback, new dict response shapes)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
5b6007b
into
chasefortier/migrate-workspaces-and-users
1 check passed
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.
User description
This PR addresses all issues identified in the code review of #20.
Changes
cometx/cli/copy.pyimport requeststo top-level (was insidetry/except Exception, causing misleading error messages whenrequestswasn't installed)self.api._get_url_server()instead ofself.api.server_url(theserver_urlproperty requires_clientand raisesAttributeError)except Exceptioninto separateHTTPErrorandRequestExceptionhandlers so the error message includes the HTTP status code when availablecometx/cli/migrate_users.py--url/--source-urlargs for self-hosted Comet instances whose API keys do not encode the server URL_resolve_server_url: add base64 padding, catch all decode errors gracefully, respect explicit URL arg with highest prioritysource_api_keyfallback: no longer silently falls back toCOMET_API_KEY—--source-api-keyis now required when--chargeback-reportis not given; warn when source and dest URL+key are identical (likely a misconfiguration)_get_existing_workspaces: handle both flat-string list and dict-list API response shapes[DRY RUN] Would add 'email' to 'ws'for each user instead of only a count--failures-outputflag (default:bulk_add_failures_by_email.json) so the output path is configurable_add_member: usejson=payloadkwarg instead ofdata=json.dumps(payload)cometx --api-key ... migrate-users(global flag position) instead ofcometx migrate-users --api-key ...tests/unit/test_migrate_users.py(new)17 unit tests covering:
_resolve_server_url— explicit URL, new-style key decoding, old-style key fallback, malformed key fallback_get_existing_workspaces— flat list and dict-list responses_add_member— success, already-member, failure, request exceptionmigrate_usersdry-run — no API calls made, per-user output printed, no-email skip, missing--source-api-keyexits cleanlyTest plan
pytest tests/unit/test_migrate_users.py -vcometx copy SOURCE DEST(workspace missing, no flag) → error with--create-workspacessuggestioncometx copy SOURCE DEST --create-workspaces(workspace missing) → workspace created, copy proceedscometx migrate-users --api-key KEY --source-api-key KEY --dry-run→ per-user preview, no API callscometx migrate-users --api-key KEY --chargeback-report /path/to/report.json→ loads from filecometx migrate-users --api-key KEY(no source key, no report) → clear error message🤖 Generated with Claude Code
Generated description
Below is a concise technical summary of the changes proposed in this PR:
graph LR migrate_users_("migrate_users"):::modified resolve_server_url_("_resolve_server_url"):::added add_member_("_add_member"):::modified REQUESTS_("REQUESTS"):::added create_workspace_("_create_workspace"):::modified migrate_users_ -- "Adds URL resolution, enforces https, errors when undetermined." --> resolve_server_url_ migrate_users_ -- "Uses add_member_url, captures status/error, writes failures." --> add_member_ add_member_ -- "Now posts JSON body using requests' json parameter." --> REQUESTS_ create_workspace_ -- "Makes workspace creation reusable; propagates HTTP errors." --> REQUESTS_ classDef added stroke:#15AA7A classDef removed stroke:#CD5270 classDef modified stroke:#EDAC4C linkStyle default stroke:#CBD5E1,font-size:13pxImprove the migrate_users CLI by mandating explicit source API credentials when needed, resolving server URLs more robustly, exposing failure output paths, and documenting/testing the argument changes alongside better per-user dry-run feedback. Strengthen the workspace-creation flow in CopyManager so it reuses the migrate-users helper, logs detailed HTTP errors, and avoids misusing the API client’s
server_urlproperty.Modified files (3)
Latest Contributors(2)
_create_workspaceand surface HTTP failures with proper URL handling.Modified files (1)
Latest Contributors(2)