refactor: remove more undocumented API usage #734
Conversation
|
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified Files
|
|
|
||
| # If name exists, append suffix and try again | ||
| if len(results) > 0: | ||
| suffix = 1 |
There was a problem hiding this comment.
I suggest we consider using a short hash for the suffix instead of an incremental number.
As there is no locking involved appending an incremental is still subject to two people getting the same unique name if they are deploying together.
An hash wouldn't absolutely prevent that, but the chances for a collision are greatly reduced.
There was a problem hiding this comment.
This is existing behavior, I wasn't trying to change it here. I can make an issue about changing it in a followup, if you'd like.
I agree that a hash would be more robust, but I'm not sure it matters actually. I saw this in the API docs:
Note: content names are unique within the owning user's account, so a request that specifies a non-empty name and owner_guid will return at most one content item.
So you would only run the risk of collision with this method if you were concurrently using the same user's credentials trying to create multiple items with the same name. And worst case, IIUC, if two concurrent jobs with the same user's credentials picked the same unique name, one of them would be rejected by the server.
Intent
Following #731, I saw some more that could be removed and thought I'd push forward. Removed here:
applications/:id/config: the server just appends things to the dashboard_url on the content record, we can do that here too.applications/search: the only place this was actually used was to detect name collisions. Swapped to use thev1/content?name=query, which has been around much longer than thev1/search/contentendpoint, and actually suits this use case better anyway.If we remove support for numeric app ids in the CLI, we can remove
applications/:idtoo. I left a comment about deprecating that. For now, it is encapsulated in only one calling site.Approach
Claude-assisted refactoring. Stopped a few times to do some GitHub code search to confirm that functions were unused outside of the package and could be safely removed.
Automated Tests
This seems to be covered by some tests. I can run the full integration tests with latest Connect too. It would be ideal to have a short test suite that can run across historical versions using
with-connect, as discussed in #649.Checklist
rsconnect-python-tests-at-nightworkflow in Connect against this feature branch.