-
Notifications
You must be signed in to change notification settings - Fork 565
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
Issue mirrorer #2578
Issue mirrorer #2578
Conversation
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.
nice!!! some initial comments
and testcase.github_issue_num is not None | ||
|
||
|
||
def get_github_issue(testcase, github_access): |
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 re naming: We can just call this get_issue
to avoid code stutter.
so callers will call github.get_issue
instead of the repetitive github.get_github_issue
. Same with other functions in this file, e.g. file_github_issue
and more.
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.
Ah, that makes sense.
Fixing now, thanks!
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.
left some minor comments
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.
Looking good! some more comments.
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.
Did a quick pass, will review more thoroughly tomorrow.
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.
nice!! LGTM with a last nit.
if not repo_url: | ||
logs.log_error('Unable to fetch the MAIN_REPO URL from job definition.') | ||
return None | ||
repo_name = repo_url.removeprefix(GITHUB_PREFIX) |
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.
Last comment: Not all main_repo values will be github. Can we add a check for this and just exit in such a a case? Log a nomal message here (not error).
Also Line 90 shouldn't be an error log either, as MAIN_REPO is not necessarily always available.
Mirror Monorail issues to the GitHub repo of the project under test. See [this doc](https://docs.google.com/document/d/11_Iozvw0Mp0qpnhOnb7Y69eVexm5wa7rSTc5JWvSVOk/edit?resourcekey=0-bxwwlTd4QVgN1f65LH9skA#) for more detail. 1. Add two properties to `FiledBug` to record the project's GitHub ID and the new issue number. 2. Parse the GitHub ID from the project's YAML file. 3. Predefine the formats of issue title&body and file/close issues with them. 4. Include `Python3` package `PyGitHub` to file/close GitHub issues. 5. Updated existing testcases to include the new `file_to_github` attribute. 6. Add testcases to verify the content of issues' title&body.
Mirror Monorail issues to the GitHub repo of the project under test. See this doc for more detail.
FiledBug
to record the project's GitHub ID and the new issue number.Python3
packagePyGitHub
to file/close GitHub issues.file_to_github
attribute.