-
Notifications
You must be signed in to change notification settings - Fork 0
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
base: github/serializers
Are you sure you want to change the base?
Conversation
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() |
There was a problem hiding this comment.
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" |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: return typing
commit_message = ( | ||
f"Update DataDoc {datadoc.id}: {datadoc.title or 'Untitled'}" | ||
) |
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
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?
d75e2ca
to
e7f1935
Compare
Context
Changes
Test Plan