Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Content repository resource #300

Merged
merged 61 commits into from
Nov 12, 2024
Merged

Conversation

zackverham
Copy link
Collaborator

@zackverham zackverham commented Sep 26, 2024

@tdstein looking for feedback here on the semantics / feel of this implementation.

If I have a test script scratch.py:

from posit.connect.client import Client

c = Client("http://localhost:3939", "<redacted>")

valid_content_guid="89c71a26-9dee-4b5a-b3af-cf135ef6079a"
# get content item
content_item = c.content.get(valid_content_guid)
# get the content item's repo
repo = content_item.repository()
print(repo)
# update something about the repo
repo.update(branch="this-is-an-updated-branch")
print(repo)
# delete the repo
repo.delete()
# error currently
print(content_item.repository())

here's what the result looks like:

(posit-sdk-py) ➜  posit-sdk-py git:(zack-192-git-repo-settings) ✗ python scratch.py
{'repository': 'https://github.com/dbkegley/rsc-sample-content', 'branch': 'main', 'directory': 'local/python-shiny', 'polling': True}
{'repository': 'https://github.com/dbkegley/rsc-sample-content', 'branch': 'this-is-an-updated-branch', 'directory': 'local/python-shiny', 'polling': True}
Traceback (most recent call last):
  File "/Users/zverham/Development/posit-dev/posit-sdk-py/scratch.py", line 16, in <module>
    print(content_item.repository())
          ^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/zverham/Development/posit-dev/posit-sdk-py/src/posit/connect/content.py", line 160, in repository
    response = self.params.session.get(url)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/zverham/Development/posit-dev/posit-sdk-py/.venv/lib/python3.12/site-packages/requests/sessions.py", line 602, in get
    return self.request("GET", url, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/zverham/Development/posit-dev/posit-sdk-py/.venv/lib/python3.12/site-packages/requests/sessions.py", line 589, in request
    resp = self.send(prep, **send_kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/zverham/Development/posit-dev/posit-sdk-py/.venv/lib/python3.12/site-packages/requests/sessions.py", line 710, in send
    r = dispatch_hook("response", hooks, r, **kwargs)
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/zverham/Development/posit-dev/posit-sdk-py/.venv/lib/python3.12/site-packages/requests/hooks.py", line 30, in dispatch_hook
    _hook_data = hook(hook_data, **kwargs)
                 ^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/zverham/Development/posit-dev/posit-sdk-py/src/posit/connect/hooks.py", line 18, in handle_errors
    raise ClientError(error_code, message, http_status, http_status_message, payload)
posit.connect.errors.ClientError: {"error_code": 4, "error_message": "The requested object does not exist.", "http_status": 404, "http_message": "Not Found", "payload": null}

Current things I'm not sure about:

  • The use of repository() as a function to get the repo information. Because we don't have a guid associated with the git info, the pattern content.repository.get() can't overload the base get function correctly, so I'm not sure the best way to express a singleton get operation that will never be parameterized.

  • If no repository exists, how do we want to represent that? The API itself returns an API error - I don't know how we want to represent this situation in the SDK.

Copy link

github-actions bot commented Sep 26, 2024

☂️ Python Coverage

current status: ✅

Overall Coverage

Lines Covered Coverage Threshold Status
1643 1513 92% 0% 🟢

New Files

File Coverage Status
src/posit/connect/_api.py 64% 🟢
src/posit/connect/_api_call.py 74% 🟢
src/posit/connect/_json.py 100% 🟢
src/posit/connect/_typing_extensions.py 78% 🟢
src/posit/connect/_utils.py 0% 🟢
TOTAL 63% 🟢

Modified Files

File Coverage Status
src/posit/connect/client.py 98% 🟢
src/posit/connect/content.py 96% 🟢
src/posit/connect/cursors.py 94% 🟢
src/posit/connect/hooks.py 100% 🟢
src/posit/connect/jobs.py 100% 🟢
src/posit/connect/oauth/integrations.py 100% 🟢
src/posit/connect/oauth/oauth.py 100% 🟢
src/posit/connect/paginator.py 95% 🟢
src/posit/connect/resources.py 85% 🟢
src/posit/connect/users.py 96% 🟢
src/posit/connect/vanities.py 94% 🟢
TOTAL 96% 🟢

updated for commit: fa21a42 by action🐍

@zackverham zackverham changed the title wip implementation of git repo resource feat: Content repository resource [draft] Sep 26, 2024
@tdstein
Copy link
Collaborator

tdstein commented Sep 27, 2024

Nice!

I've been encountering this problem as well. Since the Repository is one-to-one with the Content, the interface is different, and the existing Resource/Resources base classes do not work well with this concept.

Strictly thinking about the public interface, I agree that using repository() is weird. Instead, I would prefer to call print(content.repository) and see the response body of the GET repository endpoint. If the Repository doesn't exist, I would like to see None instead of an exception

Things get tricky when you introduce the concept of creating a Repository. I think content.create_repository(...) feels natural.

from __future__ import annotations


class Repository(dict):

    def delete(self):
        pass


class Content(dict):

    @property
    def repository(self):
        # simulated GET endpoint
        if hasattr(self, "_repository"):
            return self._repository

    def create_repository(self, **attributes):
        self._repository = Repository(**attributes)
        return self._repository


if __name__ == "__main__":
    content = Content()
    # Nothing here...
    print(content.repository)

    # Let's create one!
    repository = content.create_repository(url="git+https://github.com/posit-dev/posit-sdk-py")
    print(repository)

    # There it is!
    print(content.repository)

@zackverham
Copy link
Collaborator Author

zackverham commented Sep 30, 2024

New and improved ergonomics:

from posit.connect.client import Client

c = Client("http://localhost:3939", "<redacted>")

valid_content_guid="9086fef1-a600-4eb0-afbd-51791df32efa"

# get content item
content_item = c.content.get(valid_content_guid)
repo = content_item.repository
if repo:
    repo.update(branch="this-is-an-updated-branch")
    print(repo)

    repo.delete()
    print(content_item.repository)
else:
    print("oops, repo does not exist")

Copy link
Collaborator

@tdstein tdstein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking in to see how the Git Repository implementation is going. Looks like you have some exciting things in progress. Feel free to disregard any of my comments if they aren't relevant to where you are headed.

If making Resource immutable or revisiting attributes for fields is worth pursuing, we should split that work out from this pull request to make sure the core Git Repository functionality makes this release.

src/posit/connect/content.py Outdated Show resolved Hide resolved
src/posit/connect/content.py Outdated Show resolved Hide resolved
src/posit/connect/content.py Outdated Show resolved Hide resolved
src/posit/connect/content.py Outdated Show resolved Hide resolved
src/posit/connect/content.py Outdated Show resolved Hide resolved
src/posit/connect/content.py Outdated Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
src/posit/connect/resources.py Outdated Show resolved Hide resolved
src/posit/connect/resources.py Show resolved Hide resolved
src/posit/connect/_api.py Outdated Show resolved Hide resolved
@tdstein
Copy link
Collaborator

tdstein commented Nov 8, 2024

@schloerke - I went through and removed all getattr calls which triggered the warning in Resource: #323

Copy link
Collaborator

@tdstein tdstein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Functionality looks good. Just need few minor edits for consistency, then we can merge 👍🏻

src/posit/connect/content.py Outdated Show resolved Hide resolved
tests/posit/connect/test_content.py Outdated Show resolved Hide resolved
src/posit/connect/_api.py Outdated Show resolved Hide resolved
@schloerke schloerke merged commit f16a90c into main Nov 12, 2024
33 checks passed
@schloerke schloerke deleted the zack-192-git-repo-settings branch November 12, 2024 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants