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: Add github client for commit and versioning #8

Open
wants to merge 1 commit into
base: github/serializers
Choose a base branch
from

Conversation

zhangvi7
Copy link
Owner

Context

  • Adding Github client for GH API calls with OAuth token
  • Backend endpoints for commit/push, view all versions
  • Users need to have pygithub package installed and set config for github repo url

Changes

  • Datasource endpoints:
    • Commit/pushing datadoc
    • Retrieving all previous versions/commits of datadoc
  • GitHub Client
  • Logic:
  • Unit Test
  • Updated requirements/extra.txt for pygithub==2.4.0
  • Added config for GITHUB_REPO_URL

Test Plan

  • pytest querybook/tests/test_lib/test_github_integration/test_github_client.py
  • Manual E2E testing next PR

Initialize the GitHub client with an access token from the session.
Raises an exception if the token is not found.
"""
access_token = self._get_access_token()

Choose a reason for hiding this comment

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

instead of getting the access token inside this class, how about passing it token as parameter so that this class can be decoupled from the flask_session?

self.user = self.client.get_user()
self.github_link = github_link
self.repo = self._get_repository()
self.branch = "main"

Choose a reason for hiding this comment

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

It seems that the branch name needs to be configurable.

if not repo_url:
LOG.error("GITHUB_REPO_URL is not configured")
raise Exception("GITHUB_REPO_URL is not configured")
repo_full_name = self._extract_repo_full_name(repo_url)

Choose a reason for hiding this comment

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

if repo_url is only used to parse the repo full name, why not just provide the repo name in the config directly?

file_name = f"datadoc_{datadoc.id}.md"
return f"{directory}/{file_name}"

def commit_datadoc(self, datadoc: DataDoc, commit_message: Optional[str] = None):

Choose a reason for hiding this comment

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

nit: return typing

Comment on lines 72 to 74
commit_message = (
f"Update DataDoc {datadoc.id}: {datadoc.title or 'Untitled'}"
)

Choose a reason for hiding this comment

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

do we want to allow user to provide a custom commit message?

)
LOG.info(f"Updated file {self.file_path} in repository.")
except GithubException as e:
if e.status == 404:

Choose a reason for hiding this comment

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

if this the recommended way to check the file existence?

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.

2 participants