Choose and implement a vendoring method for dependencies so that we can pin them to specific versions such that their API changes don't break our build.

The build is currently broken on master because some of our dependencies have changed their APIs since the code was written.

This will be a blocker for issue #2.

In order to enable users to migrate to CRBot or away from it, we need to support an in-between state where some users' CLAs are being managed outside of this bot, because another tool will add a marker on the PR regarding its compliance, and we shouldn't duplicate the work, nor have two different bots/tools disagree, because that only confuses PR reviewers.

The proposed solution here is to simply add a new top-level entry external which will have the same set of entries under it, namely, people, bots, and companies, mirroring the current top-level allowed entries.

Then, migrating from another system to CRBot is as follows:

1. add all known users under the external category
2. as people, bots, or companies are migrated to be managed by CRBot, move them from external to the appropriate top-level groups
3. once external is empty, remove it, and stop running the other bot in parallel with CRBot

and migrating away from CRBot is simply the following set of steps:

1. start adding new users to external
2. migrate existing users to external
3. once the only non-empty section left is external, stop running CRBot

Once this feature is implemented, the above migration steps should be added to the docs.

Add integration with Travis CI to ensure each PR and the master branch are tested for each commit.

Gmail email address can use or omit periods anywhere in the username, i.e., the following email addresses are all equivalent and refer to the same user's mailbox:

• [email protected]
• [email protected]
• [email protected]
• ...

Code Review Bot should compare Gmail email addresses based on a canonicalization which omits periods from the left-hand side of the @.

https://goreportcard.com/report/github.com/google/code-review-bot#golint shows a number of easily-addressable lint/comment issues:

Line 18: warning: package comment should be of the form "Package ghutil ..." (golint)
Line 160: warning: exported type RepoClaLabelStatus should have comment or be unexported (golint)
Line 180: warning: exported type IssueClaLabelStatus should have comment or be unexported (golint)
Line 234: warning: exported type CommitStatus should have comment or be unexported (golint)
Line 360: warning: exported type PullRequestStatus should have comment or be unexported (golint)

Right now, the -config flag and the config file contain secrets, so it cannot be checked in, though it directs the global behavior of the tool. We should factor out the secrets into a separate config, so that general global configuration settings can be checked into a repo or shared publicly, without disclosing the secrets.

Since CRBot can add labels like cla: yes, cla: no and cla: external to a PR (if those labels exist), it would be nice to ensure both the existence of labels, as well as ensure that their colors are consistent across repos, and optionally, that they have descriptions (also consistent across repos).

If we generalize it further, it would be nice to have a tool for managing common labels across repos for ensuring consistency: that the labels exist, they have the same colors, and descriptions.

The configuration can be done in a simple language such as YAML or JSON as we don't need much in the way of programmability, and it would enable us to avoid having to manually add these labels to new repos when spinning up a new project or a subrepo of an existing organization/project.

We may be able to factor out some label-handling code from the current ghutil package into a new label management package for reuse in this new tool.

When committing code, users need to run:

git commit -s

which will add a line similar to the following at the end of the commit message:

Signed-off-by: Jane Doe <[email protected]>

Developers who don't want to remember to add -s every time they commit code can simplify it for themselves via an alias for git commit.

This check should ensure that:

• this line exists in each commit
• the name and email on this line matches the name + email of the commit itself

Although we added support for lowercase/uppercase emails in PR #24, we now have a MatchAccount() function, but ironically, ProcessCommit() has a locally-defined matchAccount() function (note the difference in case!) which does not do canonicalization of email addresses, or usernames, so it does not work correctly all the time.

We should remove matchAccount() and use the new MatchAccount() function everywhere as it does the correct canonicalization across multiple fields.

Right now, Code Review Bot simply adds a comment but does not block submission of the PR, so folks have to be careful to not accidentally merge a PR prior to all checks being satisfied.

Instead, Code Review Bot should act more like a reviewer:

• add comments with "Request changes" bit set so that it blocks merging
• once all checks are satisfied, add a comment and release the block on merge by resolving own review

Right now, we just return a single value of whether the PR is compliant (yes/no) and whether or not its CLA is externally-managed. It would be better to return structured information on a per-commit (or per-committer) basis, such that we could provide concrete feedback like:

• ✅ committer1 (c0ffee) CLA compliant
• ❌ committer2 (badf00d) CLA non-compliant
• ➖ committer3 (505050) CLA externally-managed

Or even more detailed error messages, e.g.,

• ❌ committer2 (badf00d) is on the contributors list, but name on commit does not match CLA
• ❌ committer3 (badc0de) is on the contributors list, but email on commit does not match CLA

Then, users can just click on the commit to see what was the issue and fix it, without requiring manual inspection of all of the commits.

Of course, if the user amends their PR, CRBot should recompute the analysis and do one of the following:

1. post a new comment and delete the old comment (if posted as a comment)
2. rewrite the original message (might be confusing?)
3. post a new review (marking the old one outdated or resolved, perhaps)

E.g., when the user's GitHub id is empty (typically due to email address not being in the user's profile), CRBot should specify that, instead of failing with a generic error such as:

Please verify the committer name, email, and GitHub username association are all correct and match CLA records.

This could also link to these docs or another doc about adding an email address to one's GitHub account.

See https://goreportcard.com/report/github.com/google/code-review-bot

Right now, the -pr flag doesn't seem to be parsed correctly, leading to crbot processing all open PRs in the repo anyway. This should be a simple fix.

In cases where there's a matching GitHub id in the CLA record, but a misspelling of contributor's name or email, we should provide a more specific error message to clarify what the developer needs to do to fix the issue.

Currently, when Googlebot verifies the CLA on a pull request, it just says:

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

For a non-googler, it can be hard to figure out who hasn't signed the CLA, especially for larger pull requests. Can you please add a list of people who haven't signed the pull request to the output?

See https://godoc.org/github.com/google/code-review-bot

After PR #39 refactored a bunch of methods to add *ghutil.GitHubClient as the first parameter in a number of functions, we now have new lint issues reported on our Go report card:

Line 145: warning: context.Context should be the first parameter of a function (golint)
Line 168: warning: context.Context should be the first parameter of a function (golint)
Line 186: warning: context.Context should be the first parameter of a function (golint)
Line 366: warning: context.Context should be the first parameter of a function (golint)
Line 396: warning: context.Context should be the first parameter of a function (golint)
Line 497: warning: context.Context should be the first parameter of a function (golint)

I prefer to keep the (notional) this argument that is *ghutil.GitHubClient as the first parameter of all the functions, and actually, we only need the context.Context object for calling the external methods in the go-github project, and this doesn't change throughout the lifetime of the GitHubClient object, so we can just save this parameter as a field in the struct, and access it whenever needed to call external functions.

This will both address the new lint warnings, as well as simplify the function signatures and improve readability of the code, including test code.