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
✨ Add support for Phabricator as a code review system #1884
Conversation
Integration tests success for |
Codecov Report
@@ Coverage Diff @@
## main #1884 +/- ##
==========================================
+ Coverage 51.42% 54.36% +2.93%
==========================================
Files 79 79
Lines 6709 6729 +20
==========================================
+ Hits 3450 3658 +208
+ Misses 3031 2834 -197
- Partials 228 237 +9 |
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.
Looks like we don't check for anything Gerrit or Phabricator specific in our code at all. Wonder, if we should generalize this to:
reviewPlatformCommitMsg = "ReviewedCommitMessage"
func hasCommitMsgReview() bool {
regexpMatch, reviewedBy := regexp.Match("\nReviewed By: (\S+)")
if regexpMatch {
// Log reviewedBy
}
regexpMatch, reviewedOn := regexp.Match("\nReviewed On: (\S+)")
if regexpMatch {
// Log reviewedOn
}
}
Note that above is mostly pseudo-code. @laurentsimon wdyt?
I don't mind improving it. |
Happy to generalize it. |
Looking for |
Yeah, let me actually add the additional check. |
Integration tests success for |
Integration tests success for |
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.
Thanks!
Signed-off-by: Vihang Mehta <vihang@pixielabs.ai>
…abricator Signed-off-by: Vihang Mehta <vihang@pixielabs.ai>
Signed-off-by: Vihang Mehta <vihang@pixielabs.ai>
Integration tests success for |
What kind of change does this PR introduce?
Supports the "Reviewed By" field used by Phabricator to check if a repo uses Phabricator as their code review system.
What is the current behavior?
Repos that use Phabricator are marked as not requiring code review.
What is the new behavior (if this is a feature change)?**
Which issue(s) this PR fixes
Fixes #1883
Special notes for your reviewer
Does this PR introduce a user-facing change?
For user-facing changes, please add a concise, human-readable release note to
the
release-note
(In particular, describe what changes users might need to make in their
application as a result of this pull request.)